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

VERIFIED FIXED in 2.2 S11 (1may)

Status

defect
--
minor
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: azasypkin, Assigned: azasypkin)

Tracking

({regression})

unspecified
2.2 S11 (1may)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.2+, b2g-v2.1 unaffected, b2g-v2.2 verified, b2g-master verified)

Details

(Whiteboard: [p=1])

Attachments

(2 attachments)

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.
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.
blocking-b2g: --- → 2.2?
triage: regression and confusing users.
please go ahead and fix on 2.2
blocking-b2g: 2.2? → 2.2+
Assignee: nobody → azasypkin
Status: NEW → ASSIGNED
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?
Attachment #8591730 - Flags: review?(felash)
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..
Attachment #8591730 - Flags: review?(felash)
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!
Attachment #8591730 - Flags: review?(felash)
Whiteboard: [p=1]
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 :)
Attachment #8591730 - Flags: review?(felash) → review+
Thanks for review!
Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
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
Attachment #8591730 - Flags: approval-gaia-v2.2?
Attachment #8591730 - Flags: approval-gaia-v2.2? → approval-gaia-v2.2+
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!
Flags: needinfo?(ryanvm)
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!
Attachment #8597414 - Flags: review?(schung)
Comment on attachment 8597414 [details] [review]
[gaia] azasypkin:v2.2-bug-1153808-when-thread-list-ui-ready > mozilla-b2g:v2.2

LGTM, thanks!
Attachment #8597414 - Flags: review?(schung) → review+
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
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
Whiteboard: [p=1] → [p=1][sms-sprint-2.2S11]
Whiteboard: [p=1][sms-sprint-2.2S11] → [p=1]
You need to log in before you can comment on or make changes to this bug.