If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

VERIFIED FIXED

Status

Webtools
Elmo
P2
normal
VERIFIED FIXED
4 years ago
3 years ago

People

(Reporter: Pike, Assigned: adrian)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(9 attachments, 6 obsolete attachments)

71.86 KB, image/png
Details
76.63 KB, image/png
Pike
: feedback+
gueroJeff
: feedback+
Details
77.28 KB, image/png
Details
76.96 KB, image/png
Details
101.88 KB, image/png
Details
76.77 KB, image/png
gueroJeff
: feedback+
Details
6.65 KB, patch
Pike
: review+
Details | Diff | Splinter Review
2.57 KB, patch
Pike
: review+
Details | Diff | Splinter Review
68.95 KB, image/png
Details
(Reporter)

Description

4 years ago
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.
(Reporter)

Updated

4 years ago
Priority: -- → P2
Assignee: nobody → adrian
(Assignee)

Comment 1

4 years ago
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?
(Reporter)

Comment 2

4 years ago
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?

(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?
(Reporter)

Comment 4

4 years ago
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?
(Assignee)

Comment 5

4 years ago
Created attachment 823556 [details] [diff] [review]
patch.diff

This is missing unit tests, but I'd like to get some feedback now. Am I doing it right?
Attachment #823556 - Flags: review?(l10n)
(Assignee)

Comment 6

4 years ago
Created attachment 823558 [details]
first-signoff.png

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

Comment 7

4 years ago
Created attachment 823560 [details]
first-release.png

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

Comment 8

4 years ago
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.
Attachment #823556 - Flags: review?(l10n) → review+
(Reporter)

Comment 9

4 years ago
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.
Attachment #823558 - Flags: feedback?(jbeatty)
(Reporter)

Comment 10

4 years ago
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.
Attachment #823560 - Flags: feedback?
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.
Attachment #823558 - Flags: feedback?(jbeatty)
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.
(Assignee)

Comment 13

4 years ago
(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?
(Assignee)

Comment 14

4 years ago
Created attachment 830778 [details]
first-signoff-2.png

New first sign-off window.
Attachment #830778 - Flags: feedback?(l10n)
Attachment #830778 - Flags: feedback?(jbeatty)
(Assignee)

Comment 15

4 years ago
Created attachment 830788 [details] [diff] [review]
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.
Attachment #823556 - Attachment is obsolete: true
Attachment #823558 - Attachment is obsolete: true
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?
Attachment #830778 - Flags: feedback?(jbeatty)
(Reporter)

Comment 17

4 years ago
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 ;-)
(Reporter)

Updated

4 years ago
Attachment #830778 - Flags: feedback?(l10n)
(Reporter)

Comment 18

4 years ago
marking needinfo on me and Jeff here.
Flags: needinfo?(l10n)
Flags: needinfo?(jbeatty)
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:l10n-drivers@mozilla.com 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!"
Flags: needinfo?(jbeatty)
(Reporter)

Comment 20

4 years ago
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
Flags: needinfo?(l10n)
(Assignee)

Comment 21

4 years ago
Created attachment 8366003 [details] [diff] [review]
patch.diff

New patch with more checkboxes and a mailto link. Screens incoming.
Attachment #830778 - Attachment is obsolete: true
Attachment #830788 - Attachment is obsolete: true
Attachment #8366003 - Flags: review?(l10n)
(Assignee)

Comment 22

4 years ago
Created attachment 8366006 [details]
first-signoff-31.png

What that window looks like when opened.
Attachment #8366006 - Flags: feedback?(l10n)
Attachment #8366006 - Flags: feedback?(jbeatty)
(Assignee)

Comment 23

4 years ago
Created attachment 8366008 [details]
first-signoff-32.png

You need to answer three questions to cross this bridge.
(Assignee)

Comment 24

4 years ago
Created attachment 8366009 [details]
first-signoff-33.png

"An African or European swallow?"
(Assignee)

Comment 25

4 years ago
Created attachment 8366010 [details]
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. :)
(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 on attachment 8366006 [details]
first-signoff-31.png

Just clearing the flag. See my previous comment.
Attachment #8366006 - Flags: feedback?(jbeatty) → feedback+
(Assignee)

Comment 28

4 years ago
Created attachment 8367347 [details]
first-signoff-35.png

New link style, matching the rest of the page.
(Assignee)

Comment 29

4 years ago
Created attachment 8367348 [details] [diff] [review]
patch.diff

New patch with link styling.
Attachment #8366003 - Attachment is obsolete: true
Attachment #8366003 - Flags: review?(l10n)
Attachment #8367348 - Flags: review?(l10n)
Comment on attachment 8367347 [details]
first-signoff-35.png

Cool! Thanks for updating the style :-D
Attachment #8367347 - Flags: feedback+
(Reporter)

Comment 31

4 years ago
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
Attachment #8367348 - Flags: review?(l10n) → review-
(Reporter)

Updated

4 years ago
Attachment #8366006 - Flags: feedback?(l10n) → feedback+
(Assignee)

Comment 32

4 years ago
Created attachment 8374008 [details] [diff] [review]
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?
Attachment #8367348 - Attachment is obsolete: true
Attachment #8374008 - Flags: review?(l10n)
(Reporter)

Comment 33

4 years ago
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)');
  });
}
Attachment #8374008 - Flags: review?(l10n) → review+

Comment 34

4 years ago
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

Updated

4 years ago
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
(Reporter)

Comment 35

4 years ago
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 36

4 years ago
Created attachment 8379693 [details] [diff] [review]
patch.diff

Axel, if I understood correctly, this is what you wanted me to do? (Screenshot following. )
Attachment #8379693 - Flags: review?(l10n)
(Assignee)

Comment 37

4 years ago
Created attachment 8379694 [details]
first-signoff-4.png
(Reporter)

Comment 38

4 years ago
Comment on attachment 8379693 [details] [diff] [review]
patch.diff

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

Yes, exactly, thanks. r=me.
Attachment #8379693 - Flags: review?(l10n) → review+

Comment 39

4 years ago
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

Updated

4 years ago
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED

Comment 40

4 years ago
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
(Reporter)

Comment 41

4 years ago
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.
(Reporter)

Comment 42

4 years ago
Deployed on both dev and prod, verified on both, too.
Status: RESOLVED → VERIFIED
(Reporter)

Updated

3 years ago
Attachment #823560 - Flags: feedback?
You need to log in before you can comment on or make changes to this bug.