Closed Bug 907562 Opened 8 years ago Closed 8 years ago

[Messages] the "unread" bit is not always removed when we read a thread

Categories

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

defect

Tracking

(blocking-b2g:-)

RESOLVED FIXED
blocking-b2g -

People

(Reporter: sync-1, Assigned: gnarf)

References

Details

Attachments

(4 files, 1 obsolete file)

Mozilla build ID:20130812041203
 
 DEFECT DESCRIPTION:
  It still display unread after read message from notification
 
  REPRODUCING PROCEDURES:
  1.Enter SMS,enter create new sms interface.
  2.Receive a SMS,tap the notification,read the SMS.
  3.Return to SMS app interface,find the SMS still display unread--KO 
 
  EXPECTED BEHAVIOUR:
  KO:it should display read
 
  ASSOCIATE SPECIFICATION:
 
  TEST PLAN REFERENCE:free test
 
  TOOLS AND PLATFORMS USED:SWV176+ZZ
 
  USER IMPACT:medium
 
  REPRODUCING RATE:5/5
 
  For FT PR, Please list reference mobile's behavior:
Clone from brother
Attached file PR511375.LOGCAT
Clone from brother
Attached file PR511375.LOGCAT
Clone from brother
Attached image PR511375.snapshot
blocking-b2g: --- → leo?
Assignee: nobody → gnarf37
I definitely see this bug on my device too, but not necessarily when I come from a notification.
Assignee: gnarf37 → nobody
Summary: [Buri][SMS]It still display unread after read message from notification → [Messages] the "unread" bit is not always removed when we read a thread
Note that sometimes we see the unread mark only when reloading the app.
Assignee: nobody → gnarf37
Attached patch patch v1 (obsolete) — Splinter Review
* The path for switching from #new to #thread=xxx didn't call ThreadListUI.mark
* Simplified the path differences, only removing the "slide" effect when going from #new to #thread=xxx
Attachment #793596 - Flags: review?(felash)
Comment on attachment 793596 [details] [diff] [review]
patch v1

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

::: apps/sms/test/unit/message_manager_test.js
@@ +380,5 @@
> +      });
> +      teardown(function() {
> +        MockThreads.currentId = null;
> +      });
> +      test('removes "new" class from messages', function() {

removed this assert on my local copy, it's copy/pasta from the other block, this version doesn't have this class to begin with (see setup)
Status: NEW → ASSIGNED
(In reply to Corey Frang [:gnarf] [:gnarf37] from comment #9)
> Created attachment 793596 [details] [diff] [review]
> patch v1
> 
> * The path for switching from #new to #thread=xxx didn't call
> ThreadListUI.mark

so, this would happen when:
* you receive a message from A, you don't read it
* instead, you start a new message, and you send a message to A
* then, when the message is sent, you're being "redirected" to the thread for A
=> it's not marked as read

I agree this is a small bug, but I saw this also in a lot more normal cases, where I merely opened the thread. That's why I think there could be a gecko bug as well.
There was a lying bug with "inThread" that you fixed too, good :)
Comment on attachment 793596 [details] [diff] [review]
patch v1

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

all this seems good to me, but I really would like to find the other bug that do exist. Maybe this patch should move to another bug instead.

::: apps/sms/js/message_manager.js
@@ +245,5 @@
>          break;
>        default:
>          var threadId = Threads.currentId;
>          var filter;
> +        var slide = true;

nit: I'd rather use "shouldSlide" or "willSlide"

::: apps/sms/test/unit/message_manager_test.js
@@ +279,5 @@
>      setup(function() {
>        this.sinon.spy(document.activeElement, 'blur');
> +      this.sinon.stub(ThreadUI, 'cancelEdit');
> +      this.sinon.stub(ThreadUI, 'renderMessages');
> +      this.sinon.stub(ThreadUI, 'updateHeaderData').callsArgWith(0);

better use .yieldsAsync(), because the real stuff is asynchronous.

@@ +283,5 @@
> +      this.sinon.stub(ThreadUI, 'updateHeaderData').callsArgWith(0);
> +      this.sinon.stub(ThreadListUI, 'cancelEdit');
> +      this.sinon.stub(ThreadListUI, 'mark');
> +      this.sinon.stub(ThreadUI.groupView, 'reset');
> +      this.sinon.stub(MessageManager, 'launchComposer');

it seems to me you really want spies instead of stubs here, the mocks already have empty functions.

@@ +286,5 @@
> +      this.sinon.stub(ThreadUI.groupView, 'reset');
> +      this.sinon.stub(MessageManager, 'launchComposer');
> +      this.sinon.stub(MessageManager, 'slide', function(direction, callback) {
> +        callback();
> +      });

use .yieldsAsync() too here

I'd accept .yields() instead if async stuff proves to be difficult and unreliable to test, but I actually never used sinon's async capabilities.

Thinking of this, actually I'd rather use .yield in the test to simulate the asynchronous nature of these functions, but really control when the callback is sent, and keep a synchronous test. And use only spies, as yield is part of the spy API.

@@ +287,5 @@
> +      this.sinon.stub(MessageManager, 'launchComposer');
> +      this.sinon.stub(MessageManager, 'slide', function(direction, callback) {
> +        callback();
> +      });
> +      ThreadUI.inThread = false;

better do that in ThreadUI.mSetup() and/or mTeardown()
Attachment #793596 - Flags: review?(felash)
Attached patch patch v2Splinter Review
Adresses julienw's comments.

- ThreadUI.inThread setting moved to MockThreadUI.mSetup
- Spies instead of stubs
- Stubs and assertions for callback functions broken into their own suite to preserve async
- `willSlide` instead of `slide`
Attachment #793596 - Attachment is obsolete: true
Attachment #794156 - Flags: review?(felash)
Comment on attachment 794156 [details] [diff] [review]
patch v2

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

I like the changes a lot.

still a small nit though.

r=me with the latest nit

::: apps/sms/test/unit/message_manager_test.js
@@ +346,5 @@
> +            ThreadUI.updateHeaderData.called
> +          );
> +        });
> +
> +        suite('> header data upadted', function() {

nit: 'updated'
Attachment #794156 - Flags: review?(felash) → review+
There is still a case that I reproduce 100%, even with this patch. I'll clone this bug then.
note to triagers: this patch is fixing a bug that happens rarely and is imho medium-risky (this touches how the navigation works), even if it adds a lot of unit tests to check that the navigation still happens.

Therefore I'd advice to not put leo+ on this bug. Corey, do you agree ?
It does seem to have a little risk, however it fixes two edge cases, and adds a lot of units to make sure it still does what it should.

I've got no strong opinion on the leo+'ness of this one.
https://github.com/mozilla-b2g/gaia/commit/fd494261ea0ade9546771c2a13abd121fe63e235
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
(In reply to Corey Frang [:gnarf] [:gnarf37] from comment #18)
> It does seem to have a little risk, however it fixes two edge cases, and
> adds a lot of units to make sure it still does what it should.
> 
> I've got no strong opinion on the leo+'ness of this one.

Hi, Could this patch be uplifted to v1.1?
xiupinglong> can you please comment on comment 17 ?

I think the bug you really want is bug 908657. This bug fixes only one edge ase whereas imho bug 908657 is a lot more important.
Blocks: 908657
leo- this bug as although this is a nice to have given the risk here and as this is intermittent lets get this fixed in future versions.As Julien suggests we will consider 908657 alternatively.
blocking-b2g: leo? → -
You need to log in before you can comment on or make changes to this bug.