897437 - SMS message timestamps do not display the expected strings.

Status: RESOLVED FIXED

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

Description

[Roy Collings]

User Agent: Mozilla/5.0 (X11; Linux i686) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/28.0.1500.71 Safari/537.36

Steps to reproduce:

Build: unagi-ICS.eng.v1-train.Rel0.4.Sprint12.B-167.Gecko-f94f793.Gaia-ef03c2b

Adjust the time on the device so you can send sms messages from:

- 2 months ago
- 7, 6, 5, 4, 3, 2, 1 days ago.
- today.

... then once the device time is back to 'today', view the message thread and examine the timestamps for each message.


Actual results:

1. You only get the 'day' (i.e. Monday etc...) if the sms was sent 3 days ago. If it was sent more days ago than that, you just get the date.

2. You only get the word "TODAY" when you are in the next hour after sending the sms. For example:

    send the SMS at 08:45 - current device time is 08:55 -> The word "TODAY" is not present.
    send the SMS at 08:45 - current device time is 09:01 -> The word "TODAY" is present.


Expected results:

1. Each day name should be present for a week (apart from today and yesterday - so we're expecting 5 day names, not 3).

2. "Today" should be in the timestamp with the time, even if we're still in the same hour the sms was sent.

Comment 1

[John Hammink]

QA should check if this is a regression.

Comment 2

[Alex Keybl [:akeybl]]

Adding qawanted for comment 1.

Comment 3

[Carlos Martínez Toral [:carlosmartinez]]

We have tested this several times in the past and it worked fine, it is a regression.

Comment 4

[ayman maat :maat]

Hi Roy, you flagged needinfo from me on this bug, but it is not clear what information you want me to provide. Could you let me know what you need.

Thanks

(btw as Carlos says in comment 3 - this  bug is a regression)

Comment 5

[Carlos Martínez Toral [:carlosmartinez]]

Hi Ayman, the thing is that we´re not sure if the behaviour has changed but the wireframes haven´t been updated. We should expect to see today - yesterday - 4 days with the name of the week and then dates? Is this still correct?

Comment 6

[Lukas Blakk [:lsblakk] use ?needinfo]

Will block since this is a regression in v1.1 - let's get an assignee here - since David is out passing to Julien for now - reassign as needed.

Comment 7

[Julien Wajsberg [:julienw]]

might be a regression from bug 889207.

I can take this.

Comment 8

[Julien Wajsberg [:julienw]]

FTR I see strange strings for these timestamps for a long time now (never took the time to file a bug about this) so it might not be a regression after all.

Will investigate more and report.

Comment 9

[Julien Wajsberg [:julienw]]

as Borja explained to me, we're supposed to have a timestamp before a message if it's the first or if the previous one was sent/received more than 10 minutes before. And if we have a group of messages where the in-between delay is less than 10 minutes, but the whole group spans in more than 10 minutes, then we must at least one timestamp header each 10 minutes.

Ayman, I can't find this in any wireframe. could you please tell me if I'm right here ?

Comment 10

[Julien Wajsberg [:julienw]]

Another question for Ayman.

Imagin we have a group of messages like this:

1. today 1:00
2. today 1:05
3. today 1:09
4. today 1:12

strictly speaking, we would have :

today 1:00
1.
2.
3.
today 1:12
4.

Would you be ok with this instead:

today 1:00
1.
today 1:09
2.
3.
4.

The fact is that this might be easier to implement and could also be faster. Not sure yet, but I'd like to have all the information :)
If you think it's not a good idea then I'll go on with the strict implementation.

Comment 11

[Julien Wajsberg [:julienw]]

Please read "today 1:05" in the second header in the second example, sorry.

Comment 12

[Julien Wajsberg [:julienw]]

Reading the code, I think the 10-minute-delay behaviour should be done only for "today". For the other days, we should only have one container per day.

Comment 13

[Borja Salguero [:borjasalguero]]

Roy, could you attach the document where you are based on? Thanks!

Comment 14

[Roy Collings]

Hi Borja,

I only have the test spec. for this - Carlos / Isabel, do you guys have a document that the expected results are based on for Borja?

Test spec: https://jirapdi.tid.es/browse/OWD-26854

Comment 15

[isabelrios]

Hi, please see the docs attached from where the test case was created
SMS specs pag 05 refers to Dialer_contacts call log page 11

Comment 16

[isabelrios]

Attachment: HTML5_SMS_20121212_R2S1_V8.0.pdf

Comment 17

[isabelrios]

Attachment: HTML5_Dialer_Contacts_20120620_R2S1_V2.0.pdf

Comment 18

[Julien Wajsberg [:julienw]]

regarding the STR 1, it's wrong then, as we're expecting 4 day names (not 5, not 3) so we still have a bug.

Comment 19

[Julien Wajsberg [:julienw]]

I may just have found the bug for 2.

These 2 bugs are not related to thread_ui.js at all, rather it was utils.js bugs :)

Hopefully I'll have a patch today.

Comment 20

[Julien Wajsberg [:julienw]]

The second one is actually a thread_ui.js bug, sorry ;)

Comment 21

[Julien Wajsberg [:julienw]]

Attachment: patch v1

Github PR is at https://github.com/mozilla-b2g/gaia/pull/11455

Rewrote the message container handling function. Now with lots of unit tests
covering lots of edge cases.

With this patch we're now looking for a suitable location starting by the end of the list, under the assumption that we'll get the messages in the correct order while rendering, and therefore it should be faster for big lists (we'll profile this with Gabriele to see if this is true).

This patch also does a very little bit of safe refactoring, still with unit
tests.
---
 apps/communications/manifest.webapp  |   27 ++--
 apps/sms/js/thread_list_ui.js        |    2 +-
 apps/sms/js/thread_ui.js             |  143 +++++++++++++++------
 apps/sms/js/utils.js                 |   50 ++++----
 apps/sms/test/unit/mock_l10n.js      |    2 +-
 apps/sms/test/unit/mock_utils.js     |    1 +
 apps/sms/test/unit/thread_ui_test.js |  229 ++++++++++++++++++++++++++++++++++
 apps/sms/test/unit/utils_test.js     |   84 +++++++++++--
 8 files changed, 450 insertions(+), 88 deletions(-)

Comment 22

[Julien Wajsberg [:julienw]]

Borja, Corey, first reviewer who can do this is ok for me. Rick, I'd value your feedback too.

Comment 23

[Julien Wajsberg [:julienw]]

Comment on attachment 788106 [details] [diff] [review]
patch v1

Note: I'll provide a newer patch because I found that this one has some performance penalty.

Comment 24

[Corey Ford [:coyotebush]]

Pretty sure you've got the wrong Corey. :)

Comment 25

[Julien Wajsberg [:julienw]]

Attachment: patch v2

> With this patch we're now looking for a suitable location starting by the end of the list, under the assumption that we'll get the messages in the correct order while rendering, 

So, this assumption was wrong, we actually get the messages in the reverse order, so I rewrote this bit. And the code for this is much shorter too now.

Also, I fixed the performance regressions of my previous patch, which were caused by a too frequent header text rendering. Now I'm rendering the header texts only when we display the chunks, or when the message is not hidden (ie: when we send/receive it while we see the thread).

Github PR is still at https://github.com/mozilla-b2g/gaia/pull/11455

First reviewer first + feedback appreciated from Rick :)

Comment 26

[Corey Frang [:gnarf]]

Comment on attachment 788231 [details] [diff] [review]
patch v2

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

manifest.webapp shouldn't be in this commit yeah?

--- 

Everything else looks pretty solid.

::: apps/sms/js/thread_list_ui.js
@@ +496,5 @@
>      var headerDOM = document.createElement('header');
>      // Append 'time-update' state
>      headerDOM.dataset.timeUpdate = true;
>      headerDOM.dataset.time = timestamp;
> +    headerDOM.dataset.isThread = 'true';

just me or is this not really changing anything?  it gets cast to a string anyway right?

Comment 27

[Julien Wajsberg [:julienw]]

(In reply to Corey Frang [:gnarf] [:gnarf37] from comment #26)
> Comment on attachment 788231 [details] [diff] [review]
> patch v2
> 
> Review of attachment 788231 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> manifest.webapp shouldn't be in this commit yeah?

yeah sorry ;)

> 
> --- 
> 
> Everything else looks pretty solid.
> 
> ::: apps/sms/js/thread_list_ui.js
> @@ +496,5 @@
> >      var headerDOM = document.createElement('header');
> >      // Append 'time-update' state
> >      headerDOM.dataset.timeUpdate = true;
> >      headerDOM.dataset.time = timestamp;
> > +    headerDOM.dataset.isThread = 'true';
> 
> just me or is this not really changing anything?  it gets cast to a string
> anyway right?

Yep, I don't like too much these implicit coercion but I'll revert this as I don't do more changes here.

Comment 28

[Julien Wajsberg [:julienw]]

Attachment: patch v3

Fixed the comments, the PR is updated at https://github.com/mozilla-b2g/gaia/pull/11455

Having profiled this with Gabriele, it's roughly 3 times faster than before (even if we don't see it so much because of other overhead.

Comment 29

[Rick Waldron [:rwaldron]]

Initially I had some objections to using "tagName", but then realized you were using Element-specific APIs and traversal properties, so there is no problem (this is mostly for other reviewers).

Comment 30

[Julien Wajsberg [:julienw]]

Yeah, I never exactly can choose between tagName, nodeName or localName and basically take one randomly. I wish we could have matchesSelector unprefixed at one point soon.

Comment 31

[Corey Frang [:gnarf]]

Comment on attachment 788521 [details] [diff] [review]
patch v3

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

Looks solid here!  Couple of questions though:

::: apps/sms/js/thread_ui.js
@@ +785,5 @@
> +          (oldDayTimestamp !== startOfDayTimestamp) || // new day
> +          (oldTimestamp < messageTimestamp - lastMessageDelay); // too old
> +
> +        if (shouldCreateNewBlock) {
> +          messageContainer.id = 'mc_' + Utils.getDayDate(oldTimestamp);

Feels like this might need to change the localization strings here too?

@@ +818,5 @@
> +      if (lastContainer) {
> +        var lastDay = Utils.getDayDate(lastContainer.dataset.timestamp);
> +        if (lastDay === startOfDayTimestamp) {
> +          // same day -> show only the time
> +          header.dataset.timeOnly = 'true';

Feels like this might stick around after the clock ticks past midnight?  Perhaps "timeOnly" should be determined somewhere else?

@@ +849,5 @@
>      var insertBeforeContainer;
> +    var curContainer = this.findFirstContainer();
> +
> +    while (curContainer &&
> +           curContainer.dataset.timestamp < startOfDayTimestamp) {

I know the strings probably still compare the same, but this a string comparing to a number, might want to +curContainer.dataset.timestamp to be totally sure?

::: apps/sms/js/utils.js
@@ +51,5 @@
> +      if (header.dataset.isThread === 'true') {
> +        newHeader = Utils.getHeaderDate(ts);
> +
> +      // only time
> +      } else if (header.dataset.timeOnly === 'true') {

Yeah - This sticks around longer than it should after midnight clock tick yeah? Can we just calculate if it's the same day here?

@@ +107,4 @@
>        var _ = navigator.mozL10n.get;
>        var today = Utils.getDayDate(Date.now());
>        var otherDay = Utils.getDayDate(time);
> +      this.date.shared.setTime(+time);

as long as you are moving this, can we move it under all the vars?

Comment 32

[Julien Wajsberg [:julienw]]

Comment on attachment 788521 [details] [diff] [review]
patch v3

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

::: apps/sms/js/thread_ui.js
@@ +785,5 @@
> +          (oldDayTimestamp !== startOfDayTimestamp) || // new day
> +          (oldTimestamp < messageTimestamp - lastMessageDelay); // too old
> +
> +        if (shouldCreateNewBlock) {
> +          messageContainer.id = 'mc_' + Utils.getDayDate(oldTimestamp);

Right, I need to change the timestamp to "oldDayTimestamp", thanks !

I think I can even do a failed testcase with this.

@@ +818,5 @@
> +      if (lastContainer) {
> +        var lastDay = Utils.getDayDate(lastContainer.dataset.timestamp);
> +        if (lastDay === startOfDayTimestamp) {
> +          // same day -> show only the time
> +          header.dataset.timeOnly = 'true';

Normally, when we're not today, we should _remove_ these headers, not change timeOnly (because it's still the same day as the one before).

The long term plan is bug 871432, where we'll remove these headers. In the mean time, I think it's ok to still show them when we're past midnight (as long as "today" gets changed to "yesterday" of course).

@@ +849,5 @@
>      var insertBeforeContainer;
> +    var curContainer = this.findFirstContainer();
> +
> +    while (curContainer &&
> +           curContainer.dataset.timestamp < startOfDayTimestamp) {

isn't this one of Javascript implicit rules ? ;) I can still do this to be explicit.

::: apps/sms/js/utils.js
@@ +51,5 @@
> +      if (header.dataset.isThread === 'true') {
> +        newHeader = Utils.getHeaderDate(ts);
> +
> +      // only time
> +      } else if (header.dataset.timeOnly === 'true') {

see previous comment, I think you're missing something here :) when it's midnight, all headers will need to be changed, not just this one ;)

@@ +107,4 @@
>        var _ = navigator.mozL10n.get;
>        var today = Utils.getDayDate(Date.now());
>        var otherDay = Utils.getDayDate(time);
> +      this.date.shared.setTime(+time);

Sure. It just needs to be _after_ the call to getDayDate because we're using the same shared time instance. In that case it was not strictly necessary btw but it's still cleaner.

Will move it one line below.

Comment 33

[Corey Frang [:gnarf]]

Comment on attachment 788521 [details] [diff] [review]
patch v3

Okay, I'm happy with the responses, giving you an r+ you can carry forward after the two tiny changes we agreed on.

Comment 34

[Julien Wajsberg [:julienw]]

master: e4e4b2fa63a61e97ce553dfd39c9316cf3fc4c75

Comment 35

[Julien Wajsberg [:julienw]]

Attachment: final patch

carrying r=gnarf

here is the landed patch, thanks !

Comment 36

[John Ford [:jhford] CET/CEST Berlin Time]

Uplifted e4e4b2fa63a61e97ce553dfd39c9316cf3fc4c75 to:
v1-train: 9b5d250f337280bb3ae5b1011eaa121d01d6a213

Comment 37

[Tim Guan-tin Chien [:timdream] (please needinfo)]

v1.1.0hd: 9b5d250f337280bb3ae5b1011eaa121d01d6a213

Comment 38

[sarsenyev]

After reviewing the attached doc (HTML5_Dialer_Contacts_20120620_R2S1_V2.0.pdf)
Page 11, the issue no longer reproduces on Leo B2G18, COM RIL,
Time labels are:
- Today
- Yesterday
- Then list the next four days by name
- Then list the rest of the days by date format (d/m/yy)
Where a contact has had more than one communication with the user using the same channel of communication (e.g. multiple calls from the
same phone number) these communications are grouped and the timestamp displayed is that of the last communication made within that
grouping.
Only "Today's" contacts have "timestamps" every 11 minutes interval 
"Today's" timestamp appears only once
The previous days have only one "timestamp"

Environmental  Variables:
Build ID: 20130816041203
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/09a8c5b6b287
Gaia: 6040508a535b587d4155356a47b22d7bd9ea4de7
Platform Version: 18.1
RIL Version: 01.01.00.019.190Firmware verision: D300f080

Comment 39

[Julien Wajsberg [:julienw]]

sarsenyev> this bug was about the Messages app, and not the Dialer app.

Comment 40

[sarsenyev]

sorry, for some reason "HTML5_SMS_20121212_R2S1_V8.0.pdf" file cannot be open.
but verified on SMS, as per comment 10 and 18, behavior is the same as expected result
Time labels are in SMS:
- Today
- Yesterday
- Then list the next four days by name
- Then list the rest of the days by date format (d/m/yy)

Where a contact has had more than one SMS with the user using the same channel of SMS (e.g. multiple sms's from the
same phone number) these SMS are grouped and the timestamp displayed is that of the last SMS made within that
grouping.
Only "Today's" contacts have "timestamps" every 11 minutes interval 
"Today's" timestamp appears only once
The previous days have only one "timestamp"

Environmental  Variables:
Build ID: 20130816041203
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/09a8c5b6b287
Gaia: 6040508a535b587d4155356a47b22d7bd9ea4de7
Platform Version: 18.1
RIL Version: 01.01.00.019.190Firmware verision: D300f080