Closed Bug 906753 Opened 8 years ago Closed 8 years ago

[Messages] we should see the recipients count in the header when composing a multi-recipient messag

Categories

(Firefox OS Graveyard :: Gaia::SMS, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: julienw, Assigned: evhan55)

Details

Attachments

(1 file)

STR:
* start a new message
* add some recipients

Expected:
* we should see the recipients count in the header

Actual:
* the header is empty 

see https://mozilla.app.box.com/applications/1/864518456/7994808358/1 page 7

we already have the "updateComposerHeader" that does exactly this but we're not calling it anywhere since bug 837994 landed.

However, initRecipients is doing something in a hacky way, and we should only remove this by a call to updateComposerHeader.

This is a good first bug if someone want to do this, but I'd like to see this fixed soon :)
Strange, this was working fine until some recent l10n api updates. I assumed there was a bug in the platform that would shake out.
(In reply to Julien Wajsberg [:julienw] from comment #0)
> STR:
> * start a new message
> * add some recipients
> 
> Expected:
> * we should see the recipients count in the header
> 
> Actual:
> * the header is empty 
> 
> see https://mozilla.app.box.com/applications/1/864518456/7994808358/1 page 7
> 
> we already have the "updateComposerHeader" that does exactly this but we're
> not calling it anywhere since bug 837994 landed.
> 
> However, initRecipients is doing something in a hacky way, and we should
> only remove this by a call to updateComposerHeader.

Why is it hacky? It's a very clear logic path: update the recipient count when the recipient list changes.

I suspect this line: https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/js/thread_ui.js#L261-L263 needs to be updated to use the new l10n apis
Corey, can you take a look at this? Is this addressed by your l10n updates?
The hacky part is https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/js/thread_ui.js#L257-259 ;)

You should not use the [many] and [one] yourself and let the l10n library handle this.

Rather, we should just replace lines 257-263 by a call to updateComposerHeader that does just this.

I don't remember if corey's changes change this.
(In reply to Julien Wajsberg [:julienw] from comment #4)
> The hacky part is
> https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/js/thread_ui.
> js#L257-259 ;)

Oh, right. Yes, that's gross. For some reason I thought Kaze had rid us of those...? I didn't even think of it when I was looking at it earlier.

> 
> You should not use the [many] and [one] yourself and let the l10n library
> handle this.

For some reason this wasn't working... but again, that was determined to be a bug.
> 
> Rather, we should just replace lines 257-263 by a call to
> updateComposerHeader that does just this.

Agreed.
Julien, I'm going to have Evelyn (from Bocoup) work on this
Assignee: nobody → evelyn
great !
- remove call to l10n.get
- replaced to use updateComposerHeader (per Julien W's recommendation)
- tests
    - add recipients updates composer header
    - remove recipients updates composer header
- updated mock recipients to have basic recipient remove logic
Attachment #792869 - Flags: review?(felash)
Comment on attachment 792869 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/11636

thanks a lot, this works very well !

I left some comments, please ask me review again when you're ready.
Attachment #792869 - Flags: review?(felash)
Comment on attachment 792869 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/11636

latest comments on github.

it's really "r+ with nits", please ask someone with rights to land on the PR or/and add "checkin-needed" as a keyword on this bug when you're ready to land
Attachment #792869 - Flags: review+
(note: "r+ with nits" means you can land without a new review, provided you fix the latest nits.)
Thanks Julien.  I have some responses to the latest nits which might take a bit more discussion before I am comfortable asking someone to land this.
Hi Julien, I think I have updated the test the way you wanted.  Please let me know if it's still incorrect.  If the review passes, please feel free to merge it, we are defering the merging of this ticket to you.
master: 0dd33a2ac3d99115257e80acb1ace8c2aa3ab902

thanks !

I'm asking leo for this:
* simple change
* good value for the user
* fix a long-lasting bug
* not risky because it replaces some specific and buggy code with a call to a previously existing function
* unit tests
=> should be in our next release
Status: NEW → RESOLVED
blocking-b2g: --- → leo?
Closed: 8 years ago
Resolution: --- → FIXED
moving to koi?
blocking-b2g: leo? → koi?
removing the nomination then, it's already in master.
blocking-b2g: koi? → ---
Attachment mime type: text/plain → text/x-github-pull-request
You need to log in before you can comment on or make changes to this bug.