879291 - [sms] When in edit mode tapping on a notification won't get you to the relevant message

Status: RESOLVED FIXED

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

Description

[Gabriele Svelto [:gsvelto]]

STR:
* open SMS app, open a thread
* trigger "edit mode"
* go to another app or the homescreen
* receive a SMS
* tap the notification

Expected:
* the SMS app is brought up and the new SMS is displayed in the thread

Actual:
* the new SMS is not displayed and the app remains in edit mode

This is a regression that was introduced as part of bug 874155.

Comment 1

[Leo]

@Gabriele Svelto,
Hi, I have tested the patch that you have provided for Notification in Bug:874155.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 2

[Leo]

This is happening not only in the Edit mode,Even when we try tapping the notification in the thread_list_ui screen we can able to reproduce this issue.

Comment 3

[Gabriele Svelto [:gsvelto]]

(In reply to Leo from comment #2)
> This is happening not only in the Edit mode,Even when we try tapping the
> notification in the thread_list_ui screen we can able to reproduce this
> issue.

Yes, that is caused by a side-effect of the edit problem. Once you tap the notification the app thinks it's still in edit mode even if you quit edit-mode manually so in the end you won't be able to see the message. I have a patch ready that fixes both problems that I'll attach in a few minutes.

Comment 4

[Gabriele Svelto [:gsvelto]]

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

Pointer to Github pull-request

Comment 5

[Leo]

Thanks Gabriele Svelto,
I will test this patch and update the result ASAP.

Comment 6

[Leo]

(In reply to Gabriele Svelto [:gsvelto] from comment #4)
> Created attachment 758437 [details]
> Pointer to Github pull request:
> https://github.com/mozilla-b2g/gaia/pull/10208
> 
> Pointer to Github pull-request

Able to reproduce this issue even with this patch.Even with this patch when we click on the notification,the New message screen is displayed with the received body in the Message field and the To field is empty. 
I have done some analysis on this,In activity_handler.js there is one function 
var showAction = function act_action() {
if (!threadId) {
        MessageManager.activity.body = body || null;
        MessageManager.activity.number = number || null;
        MessageManager.activity.contact = contact || null;

        // Move to new message
        window.location.hash = '#new';
        return;
      }
Every time it is entering into new message because here the condition is !threadId,even we get the threadId value we are toggling it and checking.
I hope this also may be problem.

Comment 7

[Gabriele Svelto [:gsvelto]]

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

Here's the updated patch taking into account your GitHub comments. I'll address Borja's comment at a later stage; I'll open a dedicated bug for that to clean up state transitions and add a unit-test to cover them. The Travis build shows a failure in thread_ui_test.js but I don't think it's caused by my patch and I can't seem to reproduce it. Maybe I'm missing something?

Comment 8

[Gabriele Svelto [:gsvelto]]

(In reply to Leo from comment #6)
> Able to reproduce this issue even with this patch.Even with this patch when
> we click on the notification,the New message screen is displayed with the
> received body in the Message field and the To field is empty.

This is strange, I tried tapping on notifications both while being in and out of edit mode and everything seems to work correctly. Which version of gaia are you applying this patch too? I've tried the STR from comment 1, could you post a more detailed one maybe (initial state of the SMS app, gaia commit, etc...)? Also could you post a screenshot of the problem?

> I have done some analysis on this,In activity_handler.js there is one
> function 
> var showAction = function act_action() {
> if (!threadId) {
>         MessageManager.activity.body = body || null;
>         MessageManager.activity.number = number || null;
>         MessageManager.activity.contact = contact || null;
> 
>         // Move to new message
>         window.location.hash = '#new';
>         return;
>       }
> Every time it is entering into new message because here the condition is
> !threadId,even we get the threadId value we are toggling it and checking.
> I hope this also may be problem.

That code path should trigger only if you have a new message activity (for example by tapping the compose button in the contact app); when receiving a message threadId should always be set AFAIK.

Comment 9

[Leo]

Hi Gabriele Svelto,
I have analysed the code and find out the problem for this.
Here is the fix along with you patch.
In activity_handler.js,
request.onsuccess = function onsuccess() {
      ActivityHandler.toView(message);
we are passing the message where we are not getting the proper threadid value(the threadId value is getting null).
So,instead of passing the message we have used 
ActivityHandler.toView(request.result);
The request.result is giving the correct value for the threadId in each message.
I am just giving a patch,all the scenarios are working fine with this change(along with your patch).

Comment 10

[Leo]

Attachment: Added a patch for Notification fix

Comment 11

[Leo]

Attachment: Added a patch for Notification fix

Comment 12

[Gabriele Svelto [:gsvelto]]

(In reply to Leo from comment #9)
> request.onsuccess = function onsuccess() {
>       ActivityHandler.toView(message);
> we are passing the message where we are not getting the proper threadid
> value(the threadId value is getting null).

What I find strange is that for the code to reach the line you changed with a null threadId then it means that the number is also null otherwise it wouldn't go past this check:

https://mxr.mozilla.org/gaia/source/apps/sms/js/activity_handler.js#105

AFAIK this shouldn't be happening so I would like to know the detailed STR you're using to hit this condition.

> So,instead of passing the message we have used 
> ActivityHandler.toView(request.result);

I think this change is valid but I'd like to know why it's happening; we might want to change the bug title to better describe the situation then.

Comment 13

[Gabriele Svelto [:gsvelto]]

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

I'm testing a simplified fix as per the discussion on GitHub; I'll set the review flag again when I'm done.

@Leo
Regarding the issue highlighted in comment 6 me and :gnarf discussed a bit on the IRC and we feel that it's a different problem and as such it would be better if you could open a new follow-up bug with a detailed STR. To be more specific I'd like to know:

- what is the initial state of the SMS app when the problem happens

- what is the exact sequence needed to trigger it, it's not a problem even if it's a bit verbose, the more information the better

- have a screenshot of the issue

We feel that the patch in attachment 758532 [details] [diff] [review] is the right thing to do but we'd like to know the exact cause of the problem first; so please open a new bug with the STR there and the proposed fix too.

Comment 14

[Gabriele Svelto [:gsvelto]]

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

Here's the simplified fix, I've tested all the relevant scenarios on my phone (including activities) and they all seem to work well now. The Travis build didn't seem to have hit any regressions but failed due to unrelated issues.

Comment 15

[Corey Frang [:gnarf]]

Comment on attachment 758532 [details] [diff] [review]
Added a patch for Notification fix

please put this on a new bug, I like the change though

Comment 16

[Corey Frang [:gnarf]]

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

r=me - thanks!

Comment 17

[Corey Frang [:gnarf]]

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

Comment 18

[John Ford [:jhford] CET/CEST Berlin Time]

I was not able to uplift this bug to v1-train.  If this bug has dependencies which are not marked in this bug, please comment on this bug.  If this bug depends on patches that aren't approved for v1-train, we need to re-evaluate the approval.  Otherwise, if this is just a merge conflict, you might be able to resolve it with:

  git checkout v1-train
  git cherry-pick -x  48b2ec2b6a2f40730485159f75bca60ea5423c9d
  <RESOLVE MERGE CONFLICTS>
  git commit

Comment 19

[Corey Frang [:gnarf]]

uplifted master: 48b2ec2b6a2f40730485159f75bca60ea5423c9d
to v1-train: 0930b6d924d98bde66601005813cf54102b50fa0