Page MenuHomePhabricator

"Patch-For-Review" should be updated if the status in Gerrit changes
Closed, ResolvedPublic

Description

Currently, adding Bug: T123 to the commit message of a Gerrit change will add a tag "Patch-For-Review" to the related task. Great!

However, the tag stays when the patch is not under review anymore, even because it has been merged or abandoned. What would be the right solution? Just remove the Patch-For-Review tag or change it for another one?

Event Timeline

Qgil created this task.Nov 7 2014, 4:19 PM
Qgil raised the priority of this task from to Normal.
Qgil updated the task description. (Show Details)
Qgil added a project: Phabricator.
Qgil changed Security from none to None.
Qgil added subscribers: Qgil, QChris.

Current behavior in Bugzilla is not doing anything with the 'PATCH_TO_REVIEW' status and leave it to the humans. However, it's no longer a status in Phab, so 'removing' is a more reasonable option.

Potential issue to think of when removing the tag: are there other patches which also are related to that bug still under review?

Qgil lowered the priority of this task from Normal to Low.Nov 7 2014, 4:39 PM

True, in Bugzilla the update is left for humans. Setting Low priority, then (only because I'm trying to keep as "Normal" only the tasks that require quick attention in Phabricator right after the Bugzilla migration).

the tag stays when the patch is not under review anymore, even because it has been merged or abandoned. What would be the right solution?

Very personal opinion: Waiting until we use Differential, because writing some custom code trying to keep track of the number of related changesets in Gerrit which have not yet been merged or abandoned and hence are still waiting for review does not sound like the best use of developer time. :)

Aklapper lowered the priority of this task from Low to Lowest.Nov 11 2014, 5:19 PM

I agree with Aklapper that keeping track of the number of related changes is not worth it.

If for the majority of people a task has only one related change, it wouldn't need too much coding work to drop the Patch-For-Review upon each merging/abandoning of a related change. That would do the ideal thing, if a task has only one related change. But it would be counter-intuitive, if most tasks have more than one related change.

Not sure.

Qgil closed this task as Declined.Nov 17 2014, 3:40 PM
Qgil claimed this task.

Agreed. The current behavior is sensible. The real thing will come with the activation of Differential.

jayvdb added a subscriber: jayvdb.Feb 5 2015, 1:36 AM
scfc added a subscriber: scfc.Feb 5 2015, 2:01 AM
jayvdb added a comment.Feb 5 2015, 3:02 AM

If it doesnt involve too much work, it would be nice to replicate the Bugzilla behaviour, where patch-for-review was a status. When the task is closed, the tag should be removed.

If it doesnt involve too much work, it would be nice to replicate the Bugzilla behaviour, where patch-for-review was a status.

I'm strongly against this because removing that status once we have Differential will be complicated.
(Plus discussion is offtopic here and happened in T212.)

When the task is closed, the tag should be removed.

Not sure which specific steps you perform that you consider the tag a problem for closed tasks.

If it doesnt involve too much work, it would be nice to replicate the Bugzilla behaviour, where patch-for-review was a status.

I'm strongly against this because removing that status once we have Differential will be complicated.
(Plus discussion is offtopic here and happened in T212.)

When the task is closed, the tag should be removed.

Not sure which specific steps you perform that you consider the tag a problem for closed tasks.

I dont do anything, except read emails consisting of someone else removing the tag, often long after the task has been closed.

can this be solved with herald?

also I don't think it would be too fragile for the bot to enumerate all the gerrit links on a task and check them all to see if any are still open. if none are open then remove the project.

can this be solved with herald?

No. See T630#1131896 for a screenshot of available Herald conditions and actions.

Dalba awarded a token.Apr 6 2018, 7:12 AM
Dalba added a subscriber: Dalba.
Ladsgroup changed the task status from Declined to Resolved.May 22 2019, 10:46 AM
Ladsgroup claimed this task.
Ladsgroup added a subscriber: Ladsgroup.

I wrote a script that does this. I'll finish the getting token (T224099: Request for conduit token of Phabricator_maintenance user) and then run it everywhere and put it on cronjob to run it every hour or so for patches that has been changed in the last hour.

Restricted Application added a project: User-Ladsgroup. · View Herald TranscriptMay 22 2019, 10:46 AM
awight added a subscriber: awight.May 22 2019, 10:55 AM

Fantastic, thanks x10^6!