Closed Bug 868218 Opened 11 years ago Closed 11 years ago

[MMS][User Story] Handle not-downloaded messages in thread view

Categories

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

x86
macOS
defect

Tracking

(blocking-b2g:leo+, b2g18 fixed)

RESOLVED FIXED
blocking-b2g leo+
Tracking Status
b2g18 --- fixed

People

(Reporter: gnarf, Assigned: gnarf)

References

()

Details

(Keywords: feature)

Attachments

(2 files, 2 obsolete files)

Pages 39, 44, 47, and 48 of this UX document describe how to display "not-downloaded" mms messages in the thread view.
Summary: [MMS][User Story] → [MMS][User Story] Handle not-downloaded messages in thread view
Assignee: nobody → gnarf37
blocking-b2g: --- → leo+
Priority: -- → P1
Depends on: 840055
Please block mms-userstories for all the [MMS][User Story] bugs so that it's easier to keep track of them.
Flags: in-moztrap?
Depends on: 867440
Whiteboard: [NO_UPLIFT]
Keywords: feature
Depends on: 873508
Depends on: 872725
No longer depends on: 873508
Attached patch patch (obsolete) — Splinter Review
Attachment #751298 - Flags: review?(felash)
note, this patch depends on bug 872725 's patch
Any chance we have strings/visual design for these new screens? (see the wireframes)

I used a date format we had defined in shared that displays like "05/27/2013" if you are en_US, but adapts order for other locales.  We need to define more shared date formats I think if we want to show it more like the wireframe, and I'm not really sure who to ask for those.
Flags: needinfo?(vpg)
Flags: needinfo?(dietrich)
Flags: needinfo?(aymanmaat)
Corey, you should try to reach Ayman by mail.
Comment on attachment 751298 [details] [diff] [review]
patch

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

more lots of small comments than big changes

::: apps/sms/index.html
@@ +184,5 @@
>        </a>
>        -->
>      </div>
> +
> +    <div id="messages-expired-tmpl" class="hide">

If I understand correctly, this will be used for also not-expired messages, therefore I'd like a another name for this template.

::: apps/sms/js/message_manager.js
@@ +336,5 @@
>      return this._mozMobileMessage.getMessage(id);
>    },
>  
> +  retrieveMMS: function mm_retrieveMMS(id) {
> +    return this._mozMobileMessage.retrieveMMS(id);

I'm concerned that we leak out of the abstraction here (ie: the DomRequest). I know we do this for getMessage too but still. But we can leave this for a future promise-related patch maybe.

::: apps/sms/js/thread_ui.js
@@ +35,5 @@
>    CONVERTED_MESSAGE_DURATION: 3000,
>    recipients: null,
>    init: function thui_init() {
>      var _ = navigator.mozL10n.get;
> +    var templateIds = ['contact', 'expired', 'highlight', 'message', 'recipient'];

nit: seems to be a too long line

@@ +848,5 @@
>      }
>  
> +    if (notDownloaded) {
> +      // default strings:
> +      var messageString = 'not-downloaded'

nit: you miss a semicolon at the end

@@ +851,5 @@
> +      // default strings:
> +      var messageString = 'not-downloaded'
> +      var download = 'not-downloaded-download';
> +
> +      var status = message.deliveryStatus[0];

for multi-recipient stuff, we'll have to change this, do we already have a bug for this ?

@@ +863,5 @@
> +        classNames.push('expired');
> +        messageString = 'not-downloaded-expired';
> +      }
> +
> +      console.log('not-downloaded', status);

remove the log

@@ +867,5 @@
> +      console.log('not-downloaded', status);
> +
> +      if (status === 'error') {
> +        classNames.push('error');
> +      }

nit: add an empty line here

@@ +1055,5 @@
>      }
>  
>      // Click event handlers that occur outside of a message element should be
>      // defined elsewhere.
>      if (!elems.message) {

you can do an early return for elems.bubble here, and simplify the 2 conditions below

@@ +1255,5 @@
>      }
>    },
>  
> +  retrieveMMS: function thui_retrieveMMS(id) {
> +    if (typeof id !== 'number') {

we don't need the if, just use `id = +id`. Also, to help the JIT compiler which doesn't like a single variable changing its type, we could create a new var to hold the value.

@@ +1258,5 @@
> +  retrieveMMS: function thui_retrieveMMS(id) {
> +    if (typeof id !== 'number') {
> +      id = +id;
> +    }
> +    console.log('retrieve', id);

remove this log

@@ +1262,5 @@
> +    console.log('retrieve', id);
> +
> +    var request = MessageManager.retrieveMMS(id);
> +    var messageDOM = this.container.querySelector('[data-message-id="' + id +
> +      '"]');

nit: put the selector on its own line, will make it easier to read.

@@ +1267,5 @@
> +
> +    messageDOM.classList.add('pending');
> +    messageDOM.classList.remove('error');
> +
> +    request.onsuccess = (function retrieveMMSSuccess() {

we should handle errors too.
Attachment #751298 - Flags: review?(felash)
Attached patch patch (obsolete) — Splinter Review
All comments adressed in code - patch still requires bug 872725's patch first
Attachment #751298 - Attachment is obsolete: true
Attachment #751372 - Flags: review?(felash)
Comment on attachment 751372 [details] [diff] [review]
patch

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

Stealing review. Some comments, but we would need UX input here for UI design. On the other hand, I miss some tests here.

::: apps/sms/index.html
@@ +186,5 @@
>      </div>
> +
> +    <div id="messages-not-downloaded-tmpl" class="hide">
> +      <!--
> +      <span class="not-downloaded-message">${message}</span>

Why a span?

::: apps/sms/js/thread_ui.js
@@ +848,5 @@
>  
> +    if (notDownloaded) {
> +      // default strings:
> +      var messageString = 'not-downloaded';
> +      var download = 'not-downloaded-download';

Better to use 'not-downloaded-mms' and 'expired-mms'. It's more meaningful.

@@ +851,5 @@
> +      var messageString = 'not-downloaded';
> +      var download = 'not-downloaded-download';
> +
> +      // assuming that incoming message only has one deliveryStatus
> +      var status = message.deliveryStatus[0];

We would need the input from UX for getting the design for this scenario, due to the design could differ from the original bubble.

@@ +853,5 @@
> +
> +      // assuming that incoming message only has one deliveryStatus
> +      var status = message.deliveryStatus[0];
> +      var expires = Utils.date.format.localeFormat(
> +        message.expiryDate, navigator.mozL10n.get('dateTimeFormat_%x')

Probably here we should use the same rules as in the header (check 'getHeaderDate' function). Maybe we need some UX input here.

@@ +1264,5 @@
> +    }).bind(this);
> +
> +    request.onerror = (function retrieveMMSError() {
> +      messageDOM.classList.remove('pending');
> +      messageDOM.classList.add('error');

Is this the style needed for a non-retrieved MMS? Are we going to show an exclamation here? https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/style/sms.css#L424 ? I guess that we will need to add more styles here for 'incoming' MMS with different status in 'not-downloaded' scenario.

::: apps/sms/locales/sms.en-US.properties
@@ +95,5 @@
>  mms-message          = MMS message
>  notDownloaded-title  = Message is not downloaded yet.
> +not-downloaded = I have sent you a file. It will be available until {{date}}
> +not-downloaded-expired = Sorry, message cannot be downloaded as it expired on {{date}}
> +not-downloaded-download = download

'download' instead?

@@ +96,5 @@
>  notDownloaded-title  = Message is not downloaded yet.
> +not-downloaded = I have sent you a file. It will be available until {{date}}
> +not-downloaded-expired = Sorry, message cannot be downloaded as it expired on {{date}}
> +not-downloaded-download = download
> +not-downloaded-downloading = downloading

'downloading' instead?

::: apps/sms/style/sms.css
@@ +319,5 @@
>  }
>  
> +/* hide download button via css on expired messages */
> +.message.expired button {
> +  display: none;

Probably in 'expired' we dont need to render this element, because we are going to hide it after all.
Attachment #751372 - Flags: review?(felash) → review?(fbsc)
Comment on attachment 751372 [details] [diff] [review]
patch

Review done. Let me know when the changes will be ready and I will review it again! Thanks.
Attachment #751372 - Flags: review?(fbsc)
Ideally the phone would allow the user to localize/customize the date/time format, but to the best of my knowledge it does not. 

That said, the most widely used date format in the phone is the en_US one, and indeed this is the one that is used on the inbox of the messaging app, so i would use that and ignore the wireframes simply for the sake of consistency. But it must be noted that the en_US date format that will not fly in the european market, so user localization/customization of Date/Time format needs to be addressed and Moz Product should be aware of that. 

Let me know if you need more clarification
Flags: needinfo?(aymanmaat)
Ayman, we can localize whatever you want :)
Attached patch patchSplinter Review
adresses review comments from borja
Attachment #751372 - Attachment is obsolete: true
Attachment #751687 - Flags: review?(felash)
Attachment #751687 - Flags: review?(fbsc)
note that I'll probably not review this today, but tomorrow for sure.
Re: date formatting - my recommendation is to ask someone like Fabrice or Vivien what the Gaia team has been doing for time/date localization so far, to be consistent with that.
Flags: needinfo?(vpg)
Flags: needinfo?(dietrich)
Comment on attachment 751687 [details] [diff] [review]
patch

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

::: apps/sms/js/thread_ui.js
@@ +832,5 @@
>      var classNames = ['message', message.type, delivery];
> +
> +    var notDownloaded = delivery === 'not-downloaded';
> +
> +    if (delivery === 'received' || notDownloaded) {

What do you think of creating a separate data structure to describe how to map delivery values to message direction? Something like

```
var incomingDeliveries = { received: 1, 'not-downloaded': 1 };

classNames.push(incomingDeliveries[delivery] ? 'incoming' : 'outgoing');
```

(Of course, you could define that hash outside of this method, too.) You might use an array and `indexOf` instead, or maybe a full if/else instead of a ternary, but my thinking is that any such approach would be a little more explicit/readable.

@@ +854,5 @@
> +      var messageString = 'not-downloaded-mms';
> +
> +      // assuming that incoming message only has one deliveryStatus
> +      var status = message.deliveryStatus[0];
> +      var expires = Utils.date.format.localeFormat(

How about `expiryStr` instead of `expires`? I got a little lost reading through this, between `expired`, `expires`, `expired-mms`, and `expiryDate`.
(In reply to Mike Pennisi [:jugglinmike] from comment #16)
> Comment on attachment 751687 [details] [diff] [review]
> patch
> 
> Review of attachment 751687 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: apps/sms/js/thread_ui.js
> @@ +832,5 @@
> >      var classNames = ['message', message.type, delivery];
> > +
> > +    var notDownloaded = delivery === 'not-downloaded';
> > +
> > +    if (delivery === 'received' || notDownloaded) {
> 
> What do you think of creating a separate data structure to describe how to
> map delivery values to message direction? Something like
> 
> ```
> var incomingDeliveries = { received: 1, 'not-downloaded': 1 };
> 
> classNames.push(incomingDeliveries[delivery] ? 'incoming' : 'outgoing');
> ```
> 
> (Of course, you could define that hash outside of this method, too.) You
> might use an array and `indexOf` instead, or maybe a full if/else instead of
> a ternary, but my thinking is that any such approach would be a little more
> explicit/readable.
> 

The fact is that he's reusing the "notDownloaded" var later, and that's why he's defining it here.

Maybe simply renaming "notDownloaded" to "isContentNotDownloaded" would be more readable. 

I think creating a hash or array for this is really overkill here.
(In reply to Mike Pennisi [:jugglinmike] from comment #16)

> What do you think of creating a separate data structure to describe how to
> map delivery values to message direction? Something like

I had done an indexOf in an array at first but then realized i wanted a notDownloaded boolean later and switched back to the or conditional.

> > +      var expires = Utils.date.format.localeFormat(
> 
> How about `expiryStr` instead of `expires`? I got a little lost reading
> through this, between `expired`, `expires`, `expired-mms`, and `expiryDate`.

Good call, I will change to expireFormatted before this lands
No longer blocks: 810099
Depends on: 810099
Comment on attachment 751687 [details] [diff] [review]
patch

r=me

thanks !
Attachment #751687 - Flags: review?(felash) → review+
master: 1c0188f9440de1262c7c6a0d3c75d165f7f1008d
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Attachment #751687 - Flags: review?(fbsc)
Whiteboard: [NO_UPLIFT]
v1-train: 32968bd
Flags: in-moztrap? → in-moztrap+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: