532397 - [shipping] Add diffs between any changeset and last rejected sign-off

Status: RESOLVED FIXED

(Webtools Graveyard :: Elmo, defect, P5)

Description

[Staś Małolepszy :stas]

Right now you can diff between any changeset and the most recent accepted signoff (if it exists). I'd like to suggest adding a second diff link: any changeset compared to the last rejected sign-off.

This would be helpful for checking if the localizer has fixed what was the reason of us rejecting the signoff.

If there is a rejected sign-off, this could look like this:

diff to last accepted
diff to last rejected

If there isn't, you'd only have the link that we see now (but with a better description):

diff to last accepted

Comment 1

[Axel Hecht [:Pike]]

It's an interesting question on what we intend to do with a pending signoff that's preceding a rejected sign-off. Maybe it should be an option to reject all at once? I kinda fear the situation where we have 5 opt-ins after each other.

Comment 2

[Seth Bindernagel]

Can we use some of the white space below the blue-gradient trail of changesets that shows the diff information?  Seems to be some open space their to display information.

Comment 3

[Axel Hecht [:Pike]]

... and obsolete. I just landed a patch http://hg.mozilla.org/l10n/django-site/rev/4277eb8372d0 to at least fall back to obsolete optin if there's not accepted one.

Comment 4

[Staś Małolepszy :stas]

(In reply to comment #1)
> It's an interesting question on what we intend to do with a pending signoff
> that's preceding a rejected sign-off. 

Why would you have a pending sign-off before a rejected one?

Comment 5

[Zibi Braniecki [:zbraniecki][:gandalf]]

because a localizer can change his mind and opt-in with a new changeset before we react.

Comment 6

[Staś Małolepszy :stas]

Attachment: Patch v1

Here's a patch doing what I suggested in comment 0. Not sure if Axel's comment 1 shouldn't be a separate bug.

I'll attach a screenshot of how it looks like.

Comment 7

[Staś Małolepszy :stas]

Attachment: Screenshot of /pushes/ with patch v1 applied

Comment 8

[Axel Hecht [:Pike]]

I don't think that I like the UI, nor the context.

Currently, _signoffs returns the last rejected signoff that's not obsoleted. That's not what we want in this case, IMHO. We'd only want to show the rejected diff if we're actually having a rejected signoff after the last accepted one.

I'd like to come up with better visuals for the old signoffs, too. The fact that the only way to see that there was a rejected signoff is the lack of the diff-link sounds wrong.

I'd personally prefer to have the diffs either in one column, or even completely out of the columns alltogether, we could make a small dialog asking for something to diff against or so.

Not sure.

PS: I one patch that heavily merges with this one, and one patch in my head that changes the signature and architecture of _signoffs, might be worth to do this one after that.

Comment 9

[Axel Hecht [:Pike]]

Not sure why I reassigned this to me during triage, I don't think I want to own this ad-hoc.

Comment 10

[Axel Hecht [:Pike]]

FIXED by signoff2 landing.