Page MenuHomePhabricator

Implement flagging backend
Closed, ResolvedPublic5 Story Points

Description

  • If a user is logged in, they cannot flag the same collection
  • If a user is not logged in, they cannot flag the same collection unless their IP address has changed
  • If an admin unhides a collection then the collection becomes 'protected' and the collection can no longer be autohidden (flags don't do anything).
  • If an a collection is 'protected' it can still be hidden.

Event Timeline

Jdlrobson updated the task description. (Show Details)
Jdlrobson raised the priority of this task from to Needs Triage.
Jdlrobson added a project: Gather.
Jdlrobson moved this task to Must haves on the Gather board.
Jdlrobson added a subscriber: Jdlrobson.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptApr 30 2015, 8:51 PM
Jdlrobson moved this task from Must haves to This Sprint on the Gather board.May 7 2015, 8:00 PM
Jdlrobson edited a custom field.May 11 2015, 5:47 PM
Jdlrobson moved this task from In Analysis to Ready for dev on the Gather Sprint Help! board.
Tgr claimed this task.May 14 2015, 11:50 AM

@Tgr says that he is blocked on this - let us know what the issues are so we can make it possible to work on this again :)

Ping @Tgr - what are you blocked on?

Change 212593 had a related patch set uploaded (by Gergő Tisza):
[WIP] Implement flagging backend

https://gerrit.wikimedia.org/r/212593

Tgr added a comment.May 21 2015, 8:20 PM

Ping @Tgr - what are you blocked on?

Left some comments in the commit message.

"2:51 PM <tgr> jdlrobson: if you want to test the new flagging patch without reinstalling the database you can run this snippet: https://gerrit.wikimedia.org/r/#/c/212593/24..25/schema/archive/add-gl_list-flag-columns.sql

@Tgr #1060 - Duplicate column name 'gl_perm_override'
I'm mostly concerned about us running into this issue on production right now.
Running UPDATE gather_list SET gl_perm = 0 WHERE gl_perm = 2 fixed the issue for me.

Testing patch:

When I look at it it in the database the flag count is 4 but it's not hidden...


We have a task T97667 - how would you imagine this using the above? Via a hook on the API or some other means?

Tgr added a comment.Jun 24 2015, 9:05 PM

The duplicate thing could be a side effect of testing different versions of the patch on the same database, if you ran update.php with some very old version first. At any rate, schema changes have to be deployed manually on production (first on testwiki etc) so if something goes wrong it should be easy to notice in time. Worst case beta would be broken for a short time since updates are run automatically there.

Hiding should just work (and to some extent it does - it's included in the PHPUnit tests); if it doesn't, that's a bug. Please provide the query parameters in that case and I'll check.

Tgr added a comment.Jun 25 2015, 2:25 AM

@Jdlrobson sorry, I still can't reproduce the visibility thing. Could you paste the debug output from the lists API query which produces the wrong result? On vagrant, that can be watched with tail -fn0 /vagrant/logs/mediawiki-mobilewiki-debug.log. The relevant line should start with something like Query mobilewiki (2) (slave): SELECT /* Gather\api\ApiQueryLists::execute Admin */...

Talked with @Tgr at his desk. Now everything magically is working for me.
When I visit
/w/api.php?action=query&list=lists&format=json&lstprop=public&lstids=119
I now get
"perm": "public",
"perm_override": "showlist"
and the list http://localhost:8888/w/index.php/Special:Gather/id/119 is only visible to the owner of the list
That said currently there is no visual indication it is hidden - when viewing my collections and the collection itself the collection states it is 'public'. I also get no Echo notification that the flagging has occurred.

I suggested that @Tgr returns 'flagged' true|false in the api response and makes the threshold for autoflagging configurable and off by default. We can then fix the visual indication issue in the auto flagging card T97667

Tgr added a comment.Jun 26 2015, 2:16 AM

There is now a hidden property when the list is hidden. (See the API docs for details.)

phuedx added a subscriber: phuedx.Jun 29 2015, 7:00 PM

@Jdlrobson, @Tgr: Could you provide some instructions for me to sign this off? Note that given that I'm still missing context around some of the Gather backend work, so if this is purely technical, then feel free to close it as Resolved immediately.

Derp. I thought it'd been merged.

@Tgr asked me this morning not to merge as he needs to check out how to do some schema change work. I've given permission to self merge.

In terms of signing off this we'll need to merge the above config change on labs.

Change 212593 merged by jenkins-bot:
Implement flagging backend

https://gerrit.wikimedia.org/r/212593

Change 221754 merged by jenkins-bot:
Enable Gather flagging on labs

https://gerrit.wikimedia.org/r/221754

phuedx added a comment.Jul 3 2015, 3:39 PM

If I treat the description of this task as AC, then:

  • If a user is logged in, they cannot flag the same collection
  • If a user is not logged in, they cannot flag the same collection unless their IP address has changed

In both cases if I flag a collection, then the flag button disappears. If I reload the page, then the flag button reappears. I'm able to flag a collection more than once.

  • If an admin unhides a collection then the collection becomes 'protected' and the collection can no longer be autohidden (flags don't do anything).
  • If an a collection is 'protected' it can still be hidden.

I'm not sure where/how to test these so that I can sign off on it.

Confusingly, the flag icon uses EventLogging currently.
I've setup another sub task - T104717 - to cover this.

To test the API, you'll have to use API Sandbox I'm afraid using http://localhost:8888/w/index.php/Special:ApiSandbox?useformat=desktop#action=editlist&format=json&id=119&mode=flag

Tgr added a comment.Jul 3 2015, 8:53 PM

Note that there is currently no way to prevent users from flagging multiple times - the API does not tell you that you have already flagged an item, since that would make it uncacheable, and I don't think the UX benefits justify that. Repeated flaggings are simply discarded as they happen.

Jdlrobson moved this task from This Sprint to Sub tasks on the Gather board.
phuedx closed this task as Resolved.Jul 6 2015, 6:48 PM

I'm closing this as Resolve because AFAICT this is a purely back-end task. When the front-end is wired up then the feature as a whole can be signed off, not just the back-end.