1153808 - [Messages] Thread is not marked as read (in UI only) when thread is opened from notification

Status: VERIFIED FIXED

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

Description

[Oleg Zasypkin [:azasypkin]]

It happens only when Message app isn't run when user taps on notification (app was killed by LMK or device was restarted after receiving a message).

STR:
* Receive new message;
* Open task manager and close Messages app;
* Tap on notification and wait until Thread panel is opened;
* Tap on back button and observe thread, message from the first step is belong to;

Expected result: thread isn't marked as "unread";

Actual result: thread is marked as it still has unread messages;

Internally thread is marked as read and everything will be displayed correctly when user restarts app. It likely came from the patch for bug 1112135, we try to mark-as-read thread node that hasn't been rendered yet.

Comment 1

[GitHub Autolander Bot]

Attachment: [gaia] azasypkin:bug-1153808-when-thread-list-ui-ready > mozilla-b2g:master

Comment 2

[Oleg Zasypkin [:azasypkin]]

Attached WIP patch (without tests) in case we need/want to fix it.

I believe it will be almost "automatically" fixed with the future architecture approach when views are supposed to be very loosely coupled without such direct dependencies. 

So if it's not critical for v2.2, I'd rather avoid bringing any complexity here (though patch doesn't look risky), but let's ask for triage.

Note for triagers: issue goes away if user restarts message app or exits and then enters thread once again.

Comment 3

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

triage: regression and confusing users.
please go ahead and fix on 2.2

Comment 4

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8591730 [details] [review]
[gaia] azasypkin:bug-1153808-when-thread-list-ui-ready > mozilla-b2g:master

Hey Julien,

I added unit tests + made some other tweaks in the initial patch.

What do you think about it?

Comment 5

[Julien Wajsberg [:julienw]]

Comment on attachment 8591730 [details] [review]
[gaia] azasypkin:bug-1153808-when-thread-list-ui-ready > mozilla-b2g:master

I think we need the setTimeout but otherwise it works fine.

Only think that doesn't work is:
* launch the app (with a fair amount of threads)
* enter the unread conversation asap
* press back asap
=> the conversation is not marked as read before some time

But I don't have a good idea that's also not complex..

Comment 6

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8591730 [details] [review]
[gaia] azasypkin:bug-1153808-when-thread-list-ui-ready > mozilla-b2g:master

Hey Julien,

> I think we need the setTimeout but otherwise it works fine.

I've returned "setTimeout" back with comment that explains why we need it (in a separate commit).

> Only think that doesn't work is:
> * launch the app (with a fair amount of threads)
> * enter the unread conversation asap
> * press back asap
> => the conversation is not marked as read before some time
> 
> But I don't have a good idea that's also not complex..

Yeah, I can't find reasonably simple solution for this as well. We can revisit it later if it really annoys the user.

Thanks!

Comment 7

[Julien Wajsberg [:julienw]]

Comment on attachment 8591730 [details] [review]
[gaia] azasypkin:bug-1153808-when-thread-list-ui-ready > mozilla-b2g:master

r=me

thanks ! Don't forget to squash :)

Comment 8

[Oleg Zasypkin [:azasypkin]]

Thanks for review!

Comment 9

[GitHub Autolander Bot]

Pull request has landed in master: https://github.com/mozilla-b2g/gaia/commit/a7e198fb0aa004c5eebdf7f00a4431538f54f87d

Comment 10

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8591730 [details] [review]
[gaia] azasypkin:bug-1153808-when-thread-list-ui-ready > mozilla-b2g:master

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): bug 1112135
[User impact] if declined: thread won't be marked as read, if user opens Messages from notification until user re-enters thread once again or restarts application.
[Testing completed]: yes, manual and unit test
[Risk to taking this patch] (and alternatives if risky): relatively low
[String changes made]: n/a

Comment 11

[Ryan VanderMeulen [:RyanVM]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/f06ea5c7ca524f7e42596893c71e0fac36db923f

Comment 12

[Oleg Zasypkin [:azasypkin]]

Ooops, I see that patch applied to v2.2 without conflicts, but it uses method that doesn't exist in v2.2, so so some Messages integration tests are perma-red (see bug 1148152) - I'll prepare separate patch for v2.2.

Ryan, could you please backout this patch from v2.2 for now? Sorry for the inconvenience.

Thanks!

Comment 13

[Ryan VanderMeulen [:RyanVM]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/265ca0bc9408c21fc4b25a259fcee7fb642cd06b

Comment 14

[GitHub Autolander Bot]

Attachment: [gaia] azasypkin:v2.2-bug-1153808-when-thread-list-ui-ready > mozilla-b2g:v2.2

Comment 15

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8597414 [details] [review]
[gaia] azasypkin:v2.2-bug-1153808-when-thread-list-ui-ready > mozilla-b2g:v2.2

Hey Steve,

Since Julien is on PTO on Monday, could you please help to review this v2.2-only patch? I added changes needed for v2.2 as separate commit so that you can see the difference.

Thanks!

Comment 16

[Steve Chung [:steveck]]

Comment on attachment 8597414 [details] [review]
[gaia] azasypkin:v2.2-bug-1153808-when-thread-list-ui-ready > mozilla-b2g:v2.2

LGTM, thanks!

Comment 17

[Ryan VanderMeulen [:RyanVM]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/efd66b5599e058fb8456350033e4c292b56fd0c2

Comment 18

[Pi Wei Cheng [:piwei] (inactive)]

This issue is verified fixed on Flame 3.0 and 2.2. Message thread is now correctly being marked as read after following STR.

Device: Flame 3.0
BuildID: 20150428010206
Gaia: 0636405f0844bf32451a375b2d61a2b16fe33348
Gecko: caf25344f73e
Gonk: b83fc73de7b64594cd74b33e498bf08332b5d87b
Version: 40.0a1 (3.0) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:40.0) Gecko/40.0 Firefox/40.0

Device: Flame 2.2
BuildID: 20150428002500
Gaia: 9f6b1b9082662ba2c14168fc66bb02b4df3141e5
Gecko: e79c19bf19bf
Gonk: ebad7da532429a6f5efadc00bf6ad8a41288a429
Version: 37.0 (2.2) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0