Paths

Table of Contentst

Differential D25337

Update Diff: fix dead-end when not yours
ClosedPublic
Actions

Authored by valerio.bozzolan on Jul 6 2023, 20:58.

Details

Reviewers

avivey

Group Reviewers

O1: Blessed Committers

Maniphest Tasks

T15538: Update Patch: it's dead-end if not yours

Commits

rP1e3c9bdfee76: Update Diff: fix dead-end when not yours

Summary

Fix "Update Patch" for generic Differential revisions.

You can now help a coworker to update their patch, from the web.

So, if you visit Diff 111 (not yours) and you click "Update Diff", and you paste a valid diff,
you arrive here:

https://we.phorge.it/differential/diff/222/?revisionID=111

That page as default was suggesting some Diffs that are yours. Plus, now it shows Diff 111.

Omitting the Diff from which the workflow was started made no sense and it's certainly an oversight.

Any follow-up change is welcome to modernize the selector using AphrontFormTokenizerControl.

Closes T15538

Test Plan

create a Diff on Differential with user A
click on Update Diff from user B (pasting a valid diff) and Continue

Now that patch is selected (and selectable). No dead-end anymore.

Diff Detail

Repository

rP Phorge

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

valerio.bozzolan created this revision.Jul 6 2023, 20:58

Owners added a reviewer: O1: Blessed Committers.Jul 6 2023, 20:58

Herald added subscribers: Cigaryno, Matthew, tobiaswiese, speck. · View Herald TranscriptJul 6 2023, 20:58

valerio.bozzolan requested review of this revision.Jul 6 2023, 20:58

Harbormaster completed remote builds in B652: Diff 1099.Jul 6 2023, 20:58

valerio.bozzolan added inline comments.Jul 7 2023, 04:39

src/applications/differential/controller/DifferentialDiffViewController.php
181	This is kept intentionally.

valerio.bozzolan edited the summary of this revision. (Show Details)Jul 20 2023, 13:26

So in short this just assures that the current item is shown.

This is coherent with the above comment:

If a specific revision is selected (for example, because the user is following the "Update Diff" workflow), but not present in the dropdown, try to add it to the dropdown even if it is closed

valerio.bozzolan mentioned this in T15538: Update Patch: it's dead-end if not yours.Jul 24 2023, 08:59

valerio.bozzolan retitled this revision from Differential Update Patch: when not yours, fix dead-end to Differential Update Patch: fix dead-end when not yours.Jul 25 2023, 08:53

valerio.bozzolan edited the summary of this revision. (Show Details)

valerio.bozzolan edited the test plan for this revision. (Show Details)

valerio.bozzolan retitled this revision from Differential Update Patch: fix dead-end when not yours to Update Diff: fix dead-end when not yours.Jul 25 2023, 08:59

valerio.bozzolan edited the summary of this revision. (Show Details)

valerio.bozzolan edited the test plan for this revision. (Show Details)

I don't remember how to explain things to humans

I hope the commit message is understandable

I was finally able to read the code and understand what's going on here.
The original code is strange, and the withAuthors() part in it is definitely wrong in context.

This revision is now accepted and ready to land.Jul 25 2023, 09:30

ah, the original intention for the second query was "even if the diff is closed", circa 2015. Back then, it was probably not possible to update a diff owned by someone else, so it made sense to add the withAuthor..

valerio.bozzolan mentioned this in Next Up.Jul 25 2023, 12:14

Closed by commit rP1e3c9bdfee76: Update Diff: fix dead-end when not yours. · Explain WhyJul 25 2023, 12:15

This revision was automatically updated to reflect the committed changes.

valerio.bozzolan added a commit: rP1e3c9bdfee76: Update Diff: fix dead-end when not yours.

avivey mentioned this in 2023 Week 32.Aug 11 2023, 16:31