895666 - [shipping] taking a new locale to ship should be visually different

Status: VERIFIED FIXED

(Webtools Graveyard :: Elmo, defect, P2)

Description

[Axel Hecht [:Pike]]

We just ran into a situation where the default display of the sign-off page made it look like it was just a small incremental update, but in fact, it was the first instance of shipping.

This should be more apparent, in both making the initial diff probably now show up, and in warnings on the review, possibly noting that we need follow-up actions with shipped-locales, too.

Comment 1

[[DEACTIVATED] Adrian Gaudebert]

Alright, I'm trying to make my way around this bug, but I'll need a bit more explanations. I understand that this takes place on the shipping page, and that we want to change some layout on that page, but I don't understand where and when that needs to happen. 

Could you please write some steps for me to reproduce the current behavior, and what you would expect to happen instead?

Comment 2

[Axel Hecht [:Pike]]

So, https://l10n.mozilla.org/shipping/signoffs/my/fx26 is an example of a localization that wants to ship for the first time.

https://l10n.mozilla.org/shipping/signoffs/fr/fx26 is one that's shipping for a while and is incremental.

I think we can use additional text in signoff-details.html if this is a new locale to emphasize that they should check bugs. Not sure if we should link back to the bugs section on the home page? If so, we need to make sure to use the team locale.

We'll also want a blurb in the review box reminding that this is new.

Jeff, any suggestions on the wording?

Comment 3

[Jeff Beatty [:gueroJeff]]

(In reply to Axel Hecht [:Pike] from comment #2)
> So, https://l10n.mozilla.org/shipping/signoffs/my/fx26 is an example of a
> localization that wants to ship for the first time.
> 
> https://l10n.mozilla.org/shipping/signoffs/fr/fx26 is one that's shipping
> for a while and is incremental.
> 
> I think we can use additional text in signoff-details.html if this is a new
> locale to emphasize that they should check bugs. Not sure if we should link
> back to the bugs section on the home page? If so, we need to make sure to
> use the team locale.
What is the copy currently in signoff-details.html? Could you provide a URL to view it?
> 
> We'll also want a blurb in the review box reminding that this is new.
Review box: This is first sign-off for the %locale-code localization. Are you sure you want to perform its first sign-off review?
> 
> Jeff, any suggestions on the wording?

Comment 4

[Axel Hecht [:Pike]]

https://github.com/mozilla/elmo/blob/c67343ebc8d43e2ecc837ff98c07ff4f211505d7/apps/shipping/templates/shipping/signoff-details.html#L11 has the wording:

> Technically, this looks good. You did test yourself, right?

Comment 5

[[DEACTIVATED] Adrian Gaudebert]

Attachment: patch.diff

This is missing unit tests, but I'd like to get some feedback now. Am I doing it right?

Comment 6

[[DEACTIVATED] Adrian Gaudebert]

Attachment: first-signoff.png

When a user wants to sign-off for the first time.

Comment 7

[[DEACTIVATED] Adrian Gaudebert]

Attachment: first-release.png

When a user wants to review a sign-off and there is no accepted sign-off yet.

Comment 8

[Axel Hecht [:Pike]]

Comment on attachment 823556 [details] [diff] [review]
patch.diff

Review of attachment 823556 [details] [diff] [review]:
-----------------------------------------------------------------

We should have consistent logic here, see below.

I'm going to flag Jeff with feedback requests on the actual UX on the screenshots. Thanks for attaching those, that's making the discussion easier.

::: apps/shipping/views/signoff.py
@@ +384,5 @@
>          newer = sorted(newer)
>  
> +    # check if this is the very first signoff.
> +    all_signoffs = appver.signoffs.filter(locale=lang)
> +    first = len(all_signoffs) == 0

The logic here should match what we do above.

We could pass `first` in through the URL, too.

Comment 9

[Axel Hecht [:Pike]]

Comment on attachment 823558 [details]
first-signoff.png

Jeff, what should we point at here? Something to chew on:

This would be the initial release. Check your team page for tasks to complete outside of the translation status shown here.

Comment 10

[Axel Hecht [:Pike]]

Comment on attachment 823560 [details]
first-release.png

I think we can word this better and more targeted to drivers. Jeff, something to chew on:

This would be the first release for the Burmese (my) localization. Take extra care in the review and check the team page for extra tasks.

If you're accepting this, you need to file follow-up bugs for product-details and shipped-locales. You also want to update the tracking bug.

Comment 11

[Jeff Beatty [:gueroJeff]]

Comment on attachment 823558 [details]
first-signoff.png

I like the intent of the messages here, but I don't think that both need to be displayed. The first message seems off. What would you think of something like this UX?

Sign-off button is disabled.

Message reads:

"Congratulations, this looks good! This will be the first release of this locale. Are you sure you've tested your work enough to sign off?"

Followed by a check box saying, "Yes, I have tested my l10n work." When checked, the sign-off button is then enabled and the localizer can sign off.

Comment 12

[Jeff Beatty [:gueroJeff]]

Comment on attachment 823560 [details]
first-release.png

I really like this. Will the text box be enabled if the "Accept" button is selected too? It would be great, as a reviewer, to be able to add a congratulatory message when accepting a sign-off.

Comment 13

[[DEACTIVATED] Adrian Gaudebert]

(In reply to jbeatty from comment #12)
> I really like this. Will the text box be enabled if the "Accept" button is
> selected too? It would be great, as a reviewer, to be able to add a
> congratulatory message when accepting a sign-off.

Yes, that text box is always visible in the case of a first release. Two questions though: 1. should I change the text to what :Pike wrote and 2. what kind of text do you want to display to someone who checks the Accept button?

Comment 14

[[DEACTIVATED] Adrian Gaudebert]

Attachment: first-signoff-2.png

New first sign-off window.

Comment 15

[[DEACTIVATED] Adrian Gaudebert]

Attachment: patch.diff

New patch, with new text for the first sign-off box and the "first" variable is passed via the URL to signoff_details.

Comment 16

[Jeff Beatty [:gueroJeff]]

Comment on attachment 830778 [details]
first-signoff-2.png

Yes, I like that very much. After chatting a bit more with Axel, I wonder if we couldn't shorten the copy and add more conditions to enabling the final sign off button. What about something like this:

"Congratulations! This will be the first release of this locale. To sign off, please answer these questions: 
Are you following your bugzilla component?"
Checkbox "Yes"
"Have you thoroughly tested your localization?"
Checkbox "Yes"
"Have you completed productization for your locale?"
Checkbox "Yes"
"Have you translated all necessary web parts?
Checkbox "Yes"
"Have you registered to receive usage data for your locale?"
Checkbox "Yes"

What do you think Axel?

Comment 17

[Axel Hecht [:Pike]]

My concerns are two-fold:

For one, I'd rather not re-create the old green immigration form to the US.

The other one is a bit more tactical, I'd hate to have to update this UI part each time we change the wording of the requirements or the requirements themselves.

Also, often this is Dwayne hitting the buttons, so "you" might be missing the point in practice ;-)

Comment 18

[Axel Hecht [:Pike]]

marking needinfo on me and Jeff here.

Comment 19

[Jeff Beatty [:gueroJeff]]

After chatting a bit more with Axel, the first three checkboxes would enable the final sign-off button.

"Congratulations! This will be the first release of this locale. To sign off, please answer these questions: 
Are you following your bugzilla component?"
Checkbox "Yes"
"Have you thoroughly tested your localization?"
Checkbox "Yes"
"Have you completed productization for your locale?"
Checkbox "Yes"
"Are you progressing with the translation of all necessary web parts?
Optional Checkbox "Yes"
"Would you like to receive usage data for your locale?"
mailto:[e-mail] link with subject: "Requesting localizer report for xx-XX" body: "Hello, Please send me localizer reports for the xx-XX localization of Firefox desktop. Thank you!"

Comment 20

[Axel Hecht [:Pike]]

Sorry for the lag, Adrian. One more clarification, let's make the body of that email link:

Hello,

please send me the weekly localizer reports.

Name:
e-mail:
Locale code: ab
Region code: CD

Comment 21

[[DEACTIVATED] Adrian Gaudebert]

Attachment: patch.diff

New patch with more checkboxes and a mailto link. Screens incoming.

Comment 22

[[DEACTIVATED] Adrian Gaudebert]

Attachment: first-signoff-31.png

What that window looks like when opened.

Comment 23

[[DEACTIVATED] Adrian Gaudebert]

Attachment: first-signoff-32.png

You need to answer three questions to cross this bridge.

Comment 24

[[DEACTIVATED] Adrian Gaudebert]

Attachment: first-signoff-33.png

"An African or European swallow?"

Comment 25

[[DEACTIVATED] Adrian Gaudebert]

Attachment: first-signoff-34.png

What the email looks like. 

I didn't separate the two parts of the locale because it was much easier not to, especially for locales like "an" that only has the first part. Hopefully that is not important. :)

Comment 26

[Jeff Beatty [:gueroJeff]]

(In reply to Adrian Gaudebert [:adrian] from comment #22)
> Created attachment 8366006 [details]
> first-signoff-31.png
> 
> What that window looks like when opened.

I like it! My only criticism is that the mailto link needs to be styled as a link. As it stands, there's no way of knowing that is a link, nor what the action item is from the question.

Comment 27

[Jeff Beatty [:gueroJeff]]

Comment on attachment 8366006 [details]
first-signoff-31.png

Just clearing the flag. See my previous comment.

Comment 28

[[DEACTIVATED] Adrian Gaudebert]

Attachment: first-signoff-35.png

New link style, matching the rest of the page.

Comment 29

[[DEACTIVATED] Adrian Gaudebert]

Attachment: patch.diff

New patch with link styling.

Comment 30

[Jeff Beatty [:gueroJeff]]

Comment on attachment 8367347 [details]
first-signoff-35.png

Cool! Thanks for updating the style :-D

Comment 31

[Axel Hecht [:Pike]]

Comment on attachment 8367348 [details] [diff] [review]
patch.diff

Review of attachment 8367348 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good, but I have two technical concerns still.

::: apps/shipping/static/shipping/css/signoffs.css
@@ +147,5 @@
> +    color: #0096DD;
> +}
> +#signoff_desc a:hover {
> +    color: #0073AA;
> +}

Make this a full qualified copy of sandstone-resp.css rules? Also, add to the comment where we get this from?

https://github.com/mozilla/elmo/blob/master/static/css/sandstone-resp.css#L1015

::: apps/shipping/static/shipping/js/signoffs.js
@@ +176,5 @@
> +        if (!$(this).attr('checked')) {
> +          signoff_button.attr('disabled', true);
> +        }
> +      });
> +    });

I think this code would be simpler if you just used CSS' https://developer.mozilla.org/en-US/docs/Web/CSS/:checked and parentElement.querySelector.

Also, this code might flicker

Comment 32

[[DEACTIVATED] Adrian Gaudebert]

Attachment: patch.diff

I have fixed the styling with matching CSS rules and a link to sandstone, and supposedly fixed the disabled button algorithm. What do you think Axel?

Comment 33

[Axel Hecht [:Pike]]

Comment on attachment 8374008 [details] [diff] [review]
patch.diff

Review of attachment 8374008 [details] [diff] [review]:
-----------------------------------------------------------------

I think the code below is just more streamlined.

With that, r=me.

::: apps/shipping/static/shipping/js/signoffs.js
@@ +177,5 @@
> +      else {
> +        signoff_button.attr('disabled', true);
> +      }
> +    });
> +  }

I think this code should just do without jquery:

if (needed_checkboxes.length > 0) {
  var signoff_button = document.querySelector('#add_signoff input[type=submit]');
  signoff_button.disabled = true;
  needed_checkboxes.click(function (e) {
    // disable if there are unchecked boxes left
    signoff_button.disabled = !!document.querySelector('.first_signoff_needed:not(:checked)');
  });
}

Comment 34

[[github robot]]

Commit pushed to develop at https://github.com/mozilla/elmo

https://github.com/mozilla/elmo/commit/2c6ede5c1c0861766a1cb4f1e3412f17d2534d1a
Fixes bug 895666 - Added new texts on first signoff, added checkboxes on first release. r=Pike

Comment 35

[Axel Hecht [:Pike]]

I looked at this on develop, and we need to do further work, sorry.

Currently we only show the new-sign-off questionaire when the source checks in the 
{% if good %}
part of signoff-details.html. But that's actually not the generic case, we should show the questionaire in all cases.

Sorry for not catching this in the review phase.

Comment 36

[[DEACTIVATED] Adrian Gaudebert]

Attachment: patch.diff

Axel, if I understood correctly, this is what you wanted me to do? (Screenshot following. )

Comment 37

[[DEACTIVATED] Adrian Gaudebert]

Attachment: first-signoff-4.png

Comment 38

[Axel Hecht [:Pike]]

Comment on attachment 8379693 [details] [diff] [review]
patch.diff

Review of attachment 8379693 [details] [diff] [review]:
-----------------------------------------------------------------

Yes, exactly, thanks. r=me.

Comment 39

[[github robot]]

Commit pushed to develop at https://github.com/mozilla/elmo

https://github.com/mozilla/elmo/commit/151f3aa96b7f1ad271712e23dd3159d4823d4b73
Fixes bug 895666 - Displayed first sign-off form for bad releases too. r=Pike

Comment 40

[[github robot]]

Commit pushed to develop at https://github.com/mozilla/elmo

https://github.com/mozilla/elmo/commit/7c58b1e0d025ca60567327243747996e1c335a83
bug 895666, hotfix to fix first-sign-off logic, and change localizer report to a bug, per request of metrics

Comment 41

[Axel Hecht [:Pike]]

I needed to wrap a hotfix for this, I'll deploy that tomorrow, when I'm off hotel wifi, and run tests on dev and production.

Comment 42

[Axel Hecht [:Pike]]

Deployed on both dev and prod, verified on both, too.