[sms] receiving a SMS while in "delete" mode doesn't work

VERIFIED FIXED in 1.1 QE2 (6jun)

Status

P1
blocker
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: julienw, Assigned: gsvelto)

Tracking

({regression})

unspecified
1.1 QE2 (6jun)
ARM
Gonk (Firefox OS)
regression
Dependency tree / graph
Bug Flags:
in-moztrap +

Firefox Tracking Flags

(blocking-b2g:leo+, b2g18 verified)

Details

Attachments

(1 attachment, 2 obsolete attachments)

This is Bug 833010 once again.

STR:
* open Sms app, open a thread
* trigger "edit mode"
* receive a SMS

Expected:
* the new SMS is displayed in the thread

Actual:
* the new SMS is not displayed. It's not even displayed when the user quits the edit mode

This is a regression, adding qawanted to know if this happens in v1.0.1 and v1-train.
Keywords: regression
Note that there is a notification added too, whereas it should not.

Comment 2

5 years ago
I was not able to reproduce this issue on the latest v1 and v1.0.1 builds. All SMS arrived while I was in Edit mode, and Message threads were updated with sms received. 


Unagi, Build ID: 20130520070207
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/321d5b4db55a
Gaia: b161f42c5647b37e35ba5a20f394ba40bf674d9c


Unagi, Build ID: 20130520070208
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/dbecb0c45504
Gaia: ee70d7517689116622c5125ce33e56d46dd3c948

Updated

5 years ago
Keywords: qawanted
Thanks ! Therefore this is probably a regression from the MMS patches.
(Assignee)

Updated

5 years ago
Blocks: 874441
(Assignee)

Updated

5 years ago
Assignee: nobody → gsvelto
Status: NEW → ASSIGNED
(Assignee)

Comment 4

5 years ago
I've diagnosed the problem but I still have to find the change that caused it: when in edit mode it seems that the |Threads.currentId| property is set to |null| this causes this test to fail:

https://github.com/mozilla-b2g/gaia/blob/37d895322819acff82f131452d5d0950d1cdb001/apps/sms/js/activity_handler.js#L253

In turn a regular notification ends up being dispatched instead of updating the visible thread.
(Assignee)

Comment 5

5 years ago
OK, turns out the threadId patch caused this:

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

This encodes the current thread in the hash part of the URL but the different panels within the app are implemented by using the hash part too (e.g. #edit for the edit mode). So when in edit mode Threads.currentId() returns |null| because the URL ends in #edit instead of #thread<n> as it would expect.
I knew it was too clever ;)

Gabriele, do you feel like you could fix this or should it be handled to someone else ?
(Assignee)

Comment 7

5 years ago
I'm studying the code and I'll try to come up with a fix; it shouldn't be too hard. My only gripe is what I have in mind is not going to look pretty (hint: #editthread23) ;)
(Assignee)

Comment 8

5 years ago
Created attachment 753805 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/9989

Pointer to Github pull-request
(Assignee)

Comment 9

5 years ago
Comment on attachment 753805 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/9989

This patch encodes the thread ID in the URL when in thread edit mode in a way that follows the existing logic as closely as possible. The end result is that in list edit mode the location hash will be '#edit', in the thread view '#thread=<n>' and in thread edit mode '#editthread=<n>'.

This required a few small changes to the application logic: code that dealt with edit mode now checks if the beginning of the location hash matches '#edit' instead of the whole string and reacts accordingly; similarly thread-id logic now extracts the id from the trailing thread=<n> string instead of the full #thread=<n> form.

Finally this patch fixes another issue: when leaving the app in thread-edit mode and receiving a message, tapping on the notification would drop the edit mode and not show the message even if it was in the same thread. With this patch applied the new message will show up correctly in the view.
Attachment #753805 - Flags: review?(felash)
Added a comment on the PR. Maybe we can wait that Bug 870069 lands, because the patch for that bug brings in something we could use here.
Dietrich, this is a regression due to MMS work, could you please block on this ?
Flags: needinfo?(dietrich)
(Assignee)

Comment 12

5 years ago
Created attachment 755375 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/10078

Pointer to Github pull-request
(Assignee)

Comment 13

5 years ago
Comment on attachment 755375 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/10078

Here's an alternate fix for the bug that does not use the location hash, instead the edit mode is handled internally by the ThreadUI object.
Attachment #755375 - Flags: feedback?(felash)
Comment on attachment 755375 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/10078

feedback+

let's go !
Attachment #755375 - Flags: feedback?(felash) → feedback+
(Assignee)

Comment 15

5 years ago
Comment on attachment 753805 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/9989

Obsoleting, we'll go for the other solution.
Attachment #753805 - Attachment is obsolete: true
Attachment #753805 - Flags: review?(felash)
(Assignee)

Comment 16

5 years ago
Comment on attachment 755375 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/10078

If you're OK with this version r+ it and merge at will :)
Attachment #755375 - Flags: review?(felash)
blocking-b2g: leo? → leo+
Flags: needinfo?(dietrich)
Comment on attachment 755375 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/10078

ah no, sorry, it's feedback+ because I like the approach, but r- because it doesn't work ;)

I've commented on github, please have a look and we can discuss about this tomorrow !
Attachment #755375 - Flags: review?(felash) → review-
(Assignee)

Comment 18

5 years ago
Comment on attachment 755375 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/10078

Updated pull-request as per our discussion on GitHub:
- the #edit location hash has been removed entirely
- both the thread and list UIs now handle the edit mode internally
- related parts of the code have been cleaned up to leverage the new edit mode
Attachment #755375 - Flags: review- → review?
(Assignee)

Updated

5 years ago
Attachment #755375 - Flags: review? → review?(gnarf37)
Comment on attachment 755375 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/10078

r- for now, please r? me again when you have made the follow up changes.   There are comments on the pull request https://github.com/mozilla-b2g/gaia/pull/10078
Attachment #755375 - Flags: review?(gnarf37) → review-

Updated

5 years ago
Severity: normal → blocker
Priority: -- → P1
Target Milestone: --- → 1.1 QE2 (6jun)

Updated

5 years ago
Duplicate of this bug: 878427
(Assignee)

Comment 21

5 years ago
Just a quick update on the timeline, I'm at a workshop doing presentations today but I'll work on this tonight so I should have an updated patch ready by tomorrow morning.
Thanks for the update!
(Assignee)

Comment 23

5 years ago
Comment on attachment 755375 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/10078

Here's the updated pull request. I've addressed the issues with the previous one, factorized all the code for entering/exiting the edit mode and fixed the unit tests. Unfortunately I had little time to test it on my device so I'll do that tomorrow morning.
Attachment #755375 - Flags: review- → review?(gnarf37)
Comment on attachment 755375 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/10078

r=me - thanks!
Attachment #755375 - Flags: review?(gnarf37) → review+
master: https://github.com/mozilla-b2g/gaia/commit/bd9c7ed5e3d07c36312e8f7d0d076d28261a8d28
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Comment 26

5 years ago
Unfortunately my patch broke certain out-of-edit-mode transitions (like when we tap a message in the notification panel). This is actually what I feared when I prepared the alternate patch for this problem. I'll try to develop a fix today but going forward we should address the issue of state changes in a more consistent manner to prevent this kind of problems from creeping in once more.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 27

5 years ago
Created attachment 757907 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/10183

Pointer to Github pull-request
(Assignee)

Comment 28

5 years ago
Comment on attachment 757907 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/10183

Here's another PR that fixes the regression introduced by the previous one. I'm not exactly a fan of this approach which is the reason why I had come up with attachment 753805 [details] first; however at this stage there's no better fix available. Going forward I think we should move the state transition logic into some centralized place to avoid this kind of issues were we have to duplicate state-transitioning code across all paths (and risk breaking it somewhere and making the state inconsistent).
Attachment #757907 - Flags: review?(gnarf37)
gsvelto - can you open a new bug for this regression/pull, just to make uplifting easier?  I'd hate to land a bug via two commits and have one of them get lost in the uplift?  I've tagged this with no uplift until that bug is fixed.  I know julien asked me to do the same for another bug earlier, which is why I'm asking now.

I left some comments on the pull request also.
Whiteboard: [NO_UPLIFT]
(Assignee)

Comment 30

5 years ago
(In reply to Corey Frang [:gnarf] [:gnarf37] from comment #29)
> gsvelto - can you open a new bug for this regression/pull, just to make
> uplifting easier?  I'd hate to land a bug via two commits and have one of
> them get lost in the uplift?  I've tagged this with no uplift until that bug
> is fixed.  I know julien asked me to do the same for another bug earlier,
> which is why I'm asking now.

I've created bug 879291 as a clone of this bug, I'll move all the activity there.

> I left some comments on the pull request also.

Thanks, I'll address them ASAP.
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED

Comment 31

5 years ago
@Gabriele Svelto,
Hi, I have tested the patch that you have provided for Notification related Bug.Even with the patch i am able to reproduce the issue.
STR
1.Receive 2-3 messages.
2.Click on the message from the SMS notification
3.When we click on the SMS notification of the received message,the New message screen is displayed with the received body in the Message field and the To field is empty.
Note: When we click on the Notification message which is already deleted,we are getting a notification "The message has already deleted"(This scenario works fine).
(Assignee)

Updated

5 years ago
Attachment #757907 - Attachment is obsolete: true
Attachment #757907 - Flags: review?(gnarf37)
uplifted master: bd9c7ed5e3d07c36312e8f7d0d076d28261a8d28
v1-train: d0f383776b7a1aeb13057bcf9fb9ed025d14b4c6

Updated

5 years ago
status-b2g18: --- → fixed
tracking-b2g18: ? → ---

Updated

5 years ago
Whiteboard: [NO_UPLIFT]
Flags: in-moztrap?

Comment 33

5 years ago
Created test case in MozTrap: https://moztrap.mozilla.org/manage/case/8593/
Also executed existing test case: https://moztrap.mozilla.org/manage/case/4979/

Verified fixed on:
Device: Leo phone 
Build Identifier: 20130611074722
Update channel: leo/1.1.0/nightly
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/8d0562d20324
Gaia: 8c64e19b82d4e0490a7780232d3d6bd07d0ba9ec1370937291
Git commit info: 2013-06-11 07:54:51 
OS version: 1.1.0.0-prerelease

and

Device: Unagi phone 
Build Identifier: 20130611074722
Update channel: unagi/1.1.0/beta
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/8d0562d20324
Gaia: 8c64e19b82d4e0490a7780232d3d6bd07d0ba9ec1370937291
Git commit info: 2013-06-11 07:54:51 
OS version: 1.1.0.0-prerelease
Status: RESOLVED → VERIFIED
status-b2g18: fixed → verified
Flags: in-moztrap? → in-moztrap+
You need to log in before you can comment on or make changes to this bug.