Page MenuHomePhabricator

FlaggedRevisions extension: API "sighting" of revisions
Closed, ResolvedPublic

Description

Author: matthew.britton

Description:
It would be nice if revisions could be marked as "sighted" through the API when the FlaggedRevisions extension is installed.


Version: unspecified
Severity: enhancement

Details

Reference
bz16278

Event Timeline

bzimport raised the priority of this task from to Medium.Nov 21 2014, 10:23 PM
bzimport set Reference to bz16278.

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

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.

attachment bug16278.patch ignored as obsolete

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

changed parameter approve->unapprove, simplified code for extracting flag_parameters a bit

attachment bug16278.patch ignored as obsolete

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

adapted patch to the changes of r44861

Thanks. I changed the patch accordingly.

attachment bug16278.patch ignored as obsolete

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

Another fix: Only try parser cache, when reviewing the current revision of a page

Attached:

matthew.britton wrote:

Thanks to everyone who worked on this. :)