Closed Bug 874155 Opened 8 years ago Closed 8 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

(blocking-b2g:leo+, b2g18 verified)

VERIFIED FIXED
1.1 QE2 (6jun)
blocking-b2g leo+
Tracking Status
b2g18 --- verified

People

(Reporter: julienw, Assigned: gsvelto)

References

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

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.
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
Keywords: qawanted
Thanks ! Therefore this is probably a regression from the MMS patches.
Blocks: 874441
Assignee: nobody → gsvelto
Status: NEW → ASSIGNED
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.
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 ?
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) ;)
Pointer to Github pull-request
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)
Blocks: 876350
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+
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)
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-
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?
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-
Severity: normal → blocker
Priority: -- → P1
Target Milestone: --- → 1.1 QE2 (6jun)
Duplicate of this bug: 878427
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!
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
Closed: 8 years ago
Resolution: --- → FIXED
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 → ---
Pointer to Github pull-request
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]
(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
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
@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).
Attachment #757907 - Attachment is obsolete: true
Attachment #757907 - Flags: review?(gnarf37)
uplifted master: bd9c7ed5e3d07c36312e8f7d0d076d28261a8d28
v1-train: d0f383776b7a1aeb13057bcf9fb9ed025d14b4c6
Whiteboard: [NO_UPLIFT]
Flags: in-moztrap?
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
Flags: in-moztrap? → in-moztrap+
You need to log in before you can comment on or make changes to this bug.