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

Status: VERIFIED FIXED

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

Description

[Julien Wajsberg [:julienw]]

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.

Comment 1

[Julien Wajsberg [:julienw]]

Note that there is a notification added too, whereas it should not.

Comment 2

[Angela Hubenya]

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

Comment 3

[Julien Wajsberg [:julienw]]

Thanks ! Therefore this is probably a regression from the MMS patches.

Comment 4

[Gabriele Svelto [:gsvelto]]

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.

Comment 5

[Gabriele Svelto [:gsvelto]]

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.

Comment 6

[Julien Wajsberg [:julienw]]

I knew it was too clever ;)

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

Comment 7

[Gabriele Svelto [:gsvelto]]

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) ;)

Comment 8

[Gabriele Svelto [:gsvelto]]

Attachment: Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/9989

Pointer to Github pull-request

Comment 9

[Gabriele Svelto [:gsvelto]]

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.

Comment 10

[Julien Wajsberg [:julienw]]

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.

Comment 11

[Julien Wajsberg [:julienw]]

Dietrich, this is a regression due to MMS work, could you please block on this ?

Comment 12

[Gabriele Svelto [:gsvelto]]

Attachment: Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/10078

Pointer to Github pull-request

Comment 13

[Gabriele Svelto [:gsvelto]]

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.

Comment 14

[Julien Wajsberg [:julienw]]

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

feedback+

let's go !

Comment 15

[Gabriele Svelto [:gsvelto]]

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.

Comment 16

[Gabriele Svelto [:gsvelto]]

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 :)

Comment 17

[Julien Wajsberg [:julienw]]

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 !

Comment 18

[Gabriele Svelto [:gsvelto]]

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

Comment 19

[Corey Frang [:gnarf]]

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

Comment 21

[Gabriele Svelto [:gsvelto]]

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.

Comment 22

[Corey Frang [:gnarf]]

Thanks for the update!

Comment 23

[Gabriele Svelto [:gsvelto]]

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.

Comment 24

[Corey Frang [:gnarf]]

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

r=me - thanks!

Comment 25

[Corey Frang [:gnarf]]

master: https://github.com/mozilla-b2g/gaia/commit/bd9c7ed5e3d07c36312e8f7d0d076d28261a8d28

Comment 26

[Gabriele Svelto [:gsvelto]]

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.

Comment 27

[Gabriele Svelto [:gsvelto]]

Attachment: Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/10183

Pointer to Github pull-request

Comment 28

[Gabriele Svelto [:gsvelto]]

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).

Comment 29

[Corey Frang [:gnarf]]

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.

Comment 30

[Gabriele Svelto [:gsvelto]]

(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.

Comment 31

[Leo]

@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).

Comment 32

[Corey Frang [:gnarf]]

uplifted master: bd9c7ed5e3d07c36312e8f7d0d076d28261a8d28
v1-train: d0f383776b7a1aeb13057bcf9fb9ed025d14b4c6

Comment 33

[Parul Mathur]

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