Wikimedia Bugzilla is closed!

The somewhat recent FlaggedRevision.php refactoring should make this a bit easier.

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.

(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?

(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.

(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.

Created attachment 5602 [details]
changed parameter approve->unapprove, simplified code for extracting flag_parameters a bit

(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.

Some of these changes made in r44861.

Created attachment 5603 [details]
adapted patch to the changes of r44861

Thanks. I changed the patch accordingly.

(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.

Created attachment 5604 [details]
Another fix: Only try parser cache, when reviewing the current revision of a page

Done in r44880

Thanks to everyone who worked on this. :)