Last modified: 2008-12-22 13:36:46 UTC

Wikimedia Bugzilla is closed!

Wikimedia has migrated from Bugzilla to Phabricator. Bug reports should be created and updated in Wikimedia Phabricator instead. Please create an account in Phabricator and add your Bugzilla email address to it.
Wikimedia Bugzilla is read-only. If you try to edit or create any bug report in Bugzilla you will be shown an intentional error message.
In order to access the Phabricator task corresponding to a Bugzilla report, just remove "static-" from its URL.
You could still run searches in Bugzilla or access your list of votes but bug reports will obviously not be up-to-date in Bugzilla.
Bug 16278 - FlaggedRevisions extension: API "sighting" of revisions
FlaggedRevisions extension: API "sighting" of revisions
Status: RESOLVED FIXED
Product: MediaWiki extensions
Classification: Unclassified
FlaggedRevs (Other open bugs)
unspecified
All All
: Normal enhancement with 3 votes (vote)
: ---
Assigned To: Roan Kattouw
: patch, patch-need-review
Depends on:
Blocks: 16633
  Show dependency treegraph
 
Reported: 2008-11-08 01:58 UTC by Gurch
Modified: 2008-12-22 13:36 UTC (History)
5 users (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments
patch to add ApiReview.php (action=review) (11.98 KB, patch)
2008-12-20 20:18 UTC, P.Copp
Details
changed parameter approve->unapprove, simplified code for extracting flag_parameters a bit (11.54 KB, patch)
2008-12-20 21:47 UTC, P.Copp
Details
adapted patch to the changes of r44861 (7.44 KB, patch)
2008-12-21 00:09 UTC, P.Copp
Details
Another fix: Only try parser cache, when reviewing the current revision of a page (7.04 KB, patch)
2008-12-21 10:50 UTC, P.Copp
Details

Description Gurch 2008-11-08 01:58:38 UTC
It would be nice if revisions could be marked as "sighted" through the API when the FlaggedRevisions extension is installed.
Comment 1 Aaron Schulz 2008-12-20 00:38:41 UTC
The somewhat recent FlaggedRevision.php refactoring should make this a bit easier.
Comment 2 P.Copp 2008-12-20 20:18:01 UTC
Created attachment 5601 [details]
patch to add ApiReview.php (action=review)

The proposed solution adds an API module ApiReview.php, which essentially imitates the behavior of RevisionReview::AjaxReview. To make it work, it has to make two changes to other FlaggedRevs files:
*The code within FlaggedArticle::addQuickReview to generate the template and image parameters had to be factored out to its own function to avoid code duplication.
*RevisionReview::submit had to be changed to a public function to be accessible from the api.

Caveat: The revision being reviewed has to be parsed/fetched from parser cache on API call. Don't know if this will be a perfomance problem.
Comment 3 Roan Kattouw 2008-12-20 20:39:59 UTC
(In reply to comment #2)
> Created an attachment (id=5601) [details]
> patch to add ApiReview.php (action=review)
> 
Why the ==/=== change in ApiBase.php?

> The proposed solution adds an API module ApiReview.php, which essentially
> imitates the behavior of RevisionReview::AjaxReview. To make it work, it has to
> make two changes to other FlaggedRevs files:
> *The code within FlaggedArticle::addQuickReview to generate the template and
> image parameters had to be factored out to its own function to avoid code
> duplication.
> *RevisionReview::submit had to be changed to a public function to be accessible
> from the api.
> 
I haven't reviewed the FlaggedRevs changes, Aaron should do that.

Remarks on ApiReview.php:
* You're doing an awful lot of permissions checking and validation that's probably duplicated in RevisionView::AjaxReview(). Instead of duplicating this code, it should be moved to a backend function called by both AjaxReview and ApiReview.
* Use a boolean parameter instead of a 0/1 parameter for approve
** You can do this with 'approve' => false
** Setting &approve=anything produces $params['approve'] === true, not setting &approve produces $params['approve'] === false
* The variable parameter thing kind of gives me the creeps here. It would be nicer to replace all the flag_* parameters by one multivalue parameter 'flags', with a variable list of allowed values:

'flags' => array(
        ApiBase :: PARAM_ISMULTI => true,
        ApiBase :: PARAM_TYPE => $allowed_flags
),

where $allowed_flags is an array of allowed flags. You can then get these by iterating over $params['flags'], or by using $flags = array_flip($params['flags']); and using if(isset($flags['foo']))

> Caveat: The revision being reviewed has to be parsed/fetched from parser cache
> on API call. Don't know if this will be a perfomance problem.
> 
It looks kind of pointless to me: you're only parsing the page to see which images and templates are used, right? Why not get that info from the imagelinks and templatelinks tables? Does the existing FlaggedRevs code parse stuff too?
Comment 4 P.Copp 2008-12-20 21:01:17 UTC
(In reply to comment #3)
> Why the ==/=== change in ApiBase.php?
Forgot to mention it. Currently, if a parameter has array( 0, ...) as parameter type, the description says "Can be empty or..." instead of showing 0 as possible value.
 
> * You're doing an awful lot of permissions checking and validation that's
> probably duplicated in RevisionView::AjaxReview(). Instead of duplicating this
> code, it should be moved to a backend function called by both AjaxReview and
> ApiReview.
True. I first considered using AjaxReview directly, but that would probably have been even more ugly.

> * Use a boolean parameter instead of a 0/1 parameter for approve
> ** You can do this with 'approve' => false
> ** Setting &approve=anything produces $params['approve'] === true, not setting
> &approve produces $params['approve'] === false
Could do that, but would have to cast it to an integer later anyway, as RevisionReview expects 0 or 1 as value.

> * The variable parameter thing kind of gives me the creeps here. It would be
> nicer to replace all the flag_* parameters by one multivalue parameter 'flags',
> with a variable list of allowed values:
> 
> 'flags' => array(
>         ApiBase :: PARAM_ISMULTI => true,
>         ApiBase :: PARAM_TYPE => $allowed_flags
> ),
> 
> where $allowed_flags is an array of allowed flags. You can then get these by
> iterating over $params['flags'], or by using $flags =
> array_flip($params['flags']); and using if(isset($flags['foo']))
Hmm, I don't quite get that. Let's say we have 3 flags named A, B and C. A can have the values 0 or 1, B can have 0-2 and C 0-3. How would that work with multivalue params? Do you mean like it is done in ApiProtect and the expiry parameters?

> > Caveat: The revision being reviewed has to be parsed/fetched from parser cache
> > on API call. Don't know if this will be a perfomance problem.
> > 
> It looks kind of pointless to me: you're only parsing the page to see which
> images and templates are used, right? Why not get that info from the imagelinks
> and templatelinks tables? Does the existing FlaggedRevs code parse stuff too?
The existing FlaggedRevs code can take the parameters from the parser output, as it displays a from on the already parsed page. I don't know if one could get all needed parameters from the *links tables, will have to ask Aaron about it.
Comment 5 P.Copp 2008-12-20 21:21:02 UTC
(In reply to comment #4)
> > It looks kind of pointless to me: you're only parsing the page to see which
> > images and templates are used, right? Why not get that info from the imagelinks
> > and templatelinks tables? Does the existing FlaggedRevs code parse stuff too?
> The existing FlaggedRevs code can take the parameters from the parser output,
> as it displays a form on the already parsed page. I don't know if one could get
> all needed parameters from the *links tables, will have to ask Aaron about it.
On second thought, we can't take the data from the *links tables, because a) they aren't always consistent and b) we can also flag old revisions, for which the data of the *link tables is obviously incorrect.
Comment 6 P.Copp 2008-12-20 21:47:13 UTC
Created attachment 5602 [details]
changed parameter approve->unapprove, simplified code for extracting flag_parameters a bit
Comment 7 Roan Kattouw 2008-12-20 21:50:08 UTC
(In reply to comment #6)
> Created an attachment (id=5602) [details]
> changed parameter approve->unapprove, simplified code for extracting
> flag_parameters a bit
> 

The API part looks sane, but of course a lot of stuff should still be pushed to the backend.
Comment 8 Aaron Schulz 2008-12-20 23:50:47 UTC
Some of these changes made in r44861.
Comment 9 P.Copp 2008-12-21 00:09:24 UTC
Created attachment 5603 [details]
adapted patch to the changes of r44861

Thanks. I changed the patch accordingly.
Comment 10 Roan Kattouw 2008-12-21 00:21:22 UTC
(In reply to comment #4)
> (In reply to comment #3)
> > Why the ==/=== change in ApiBase.php?
> Forgot to mention it. Currently, if a parameter has array( 0, ...) as parameter
> type, the description says "Can be empty or..." instead of showing 0 as
> possible value.

Fixed in r44864.
Comment 11 P.Copp 2008-12-21 10:50:26 UTC
Created attachment 5604 [details]
Another fix: Only try parser cache, when reviewing the current revision of a page
Comment 12 Aaron Schulz 2008-12-21 21:02:44 UTC
Done in r44880
Comment 13 Gurch 2008-12-22 13:36:46 UTC
Thanks to everyone who worked on this. :)

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links