Closed Bug 837029 Opened 8 years ago Closed 7 years ago

[SMS] Messages app attempts to show deleted SMS if new message notification still exists

Categories

(Core :: DOM: Device Interfaces, defect, P1)

ARM
Gonk (Firefox OS)
defect

Tracking

()

VERIFIED FIXED
1.1 QE1 (5may)
blocking-b2g leo+
Tracking Status
b2g18 --- verified

People

(Reporter: rwood, Assigned: ssaroha)

References

()

Details

(Whiteboard: [QE1][TD-10058][ui-review})

Attachments

(3 files)

When you receive a new SMS message you get the notification for the new message in the notifications area at the top of the screen. When you have a new message notification, by default when you start the Messages app, the app automatically opens to the new message that was received as indicated by the notification.

The problem is that if you have a newly received SMS and therefore have the new message notification, then use the WebAPI to delete all SMS messages, the new message notification still remains even though the actual message(s) have been removed.  Then the next time the Messages app is started up, the app tries to display the SMS that was pointed to by the notification, and the SMS doesn't exist - leaving the Message app showing a blank screen with the phone number of the incoming SMS from the previous notification.

I believe that is a bug in the Messages app.  When the Message app starts, if there are outstanding new SMS message notifications, they should be validated to make sure the message still exists before trying to display it. This situation would also happen if a different app deletes any new SMS messages (but the new SMS notification is still there), then our Messages app is started up.

This issue was found via Gaia UI test issue 300:
https://github.com/mozilla/gaia-ui-tests/pull/300
This is related with Gecko, due to we have no way for managing previously created notifications. Updating the bug...
Component: Gaia::SMS → DOM: Device Interfaces
Product: Boot2Gecko → Core
QA Contact: mbarone976
Depends on: 779213
Duplicate of this bug: 859243
This issue has been identified as a QE1 blocker -- nominating for Leo.
blocking-b2g: --- → leo?
Whiteboard: [QE1]
Whiteboard: [QE1] → [QE1][TD-10058]
What is the detailed UX design here ? 
Should we display an information message to the user in this case ?
Flags: needinfo?(aymanmaat)
Target Milestone: --- → Leo QE1 (5may)
Please see https://bugzilla.mozilla.org/show_bug.cgi?id=859243#c3 for one very low risk fix. We're looking for a very simple fix here, rather than making any changes to the notifications.
blocking-b2g: leo? → leo+
Assignee: nobody → fernando.campo
Yep, we don't want to change the notification here, rather say something to the user when he tries to open that sms app. That's why Ayman was NEEDINFOed.
Based on the conversation in the dependent bug which proposes implementing notification.close(),

the comment#6 from :marshall_law piqued my interest
https://bugzilla.mozilla.org/show_bug.cgi?id=779213#c6

"It would have to be a Gaia-only workaround for now, i.e. expose a close() API (or similar) in Gaia's Notifications API, and Gaia apps could use this."

I was wondering how we can expose the close API only at Gaia layer without help from underlying gecko layer. 

Right now the Gaia layer code in shared/js/Notificationhelper.js has access to the list of notifications in the _referencesArray, but there is no way to identify which of these notifications stored in the _referencesArray are SMS specific. If there was someway to identify the notification type, then we could potentially have solved this problem by keeping a map of notifications with key being "notification type". So when all SMS messages are deleted by the user, we could call NotificationHelper.close('SMS'). Until that kind of meta information is exposed in notification object, I am not sure how exposing close api on gaia layer can help.

Short of changing the api at Gecko level, I agree the only other option is to show an alert dialog when the user clicks on notification for which the underlying message is no more there. this would be the fastest, albeit not the most optimal.
satender, in the NotificationHelper object loaded by the sms app, the notifications are only from SMS. This object is local to the SMS app and is used as a "proxy" to the actual notification API. You can't call close on it without having the support in the API.

I believe we keep the references in _referencesArray to prevent them from being garbage collected (but I'm not sure of that).

So the only way to fix this bug _now_ is with an alert dialog or a banner message.
Thanks Julien for the explanation. I have taken a stab to fix it via alert dialog, here is the PR: https://github.com/mozilla-b2g/gaia/pull/9276
borja, feel free to steal that review if you want it, I'm still pretty busy with tef+ stuff.
Flags: needinfo?(fbsc)
Comment on attachment 739051 [details] [review]
PR to fix case when SMS app attempts to show deleted message from new message notification

I don't remember if we need l10n reviews for leo+ bugs, so asking it here.
Attachment #739051 - Flags: review?(l10n)
Comment on attachment 739051 [details] [review]
PR to fix case when SMS app attempts to show deleted message from new message notification

Review isn't necessary for other than technical questions. For copy review, you should flag ui-review, stas posted details to .gaia.

feedback+ from me, though, looks OK for master and v1-train.
Attachment #739051 - Flags: review?(l10n) → feedback+
Hi,

Tested the pull request for this defect.
This is giving problem sometimes like,
The id which was declared is giving as '0' always. When ever the id is '0' the alert is showing for all the messages even if it is existing or not.

Checked in Gaia revision:6675a6d7c04842379a327eaa965c5d47222316a0.

Will recheck this again.

Thanks,
Leo
Priority: -- → P2
I dont know if this path it's 'hiding' a huge Gecko problem, that it's the Notification API. We need Gecko side work to have, at least, a 'notification.delete()'. Joe, Do you know who could help us?
This bug it's the same as:
https://bugzilla.mozilla.org/show_bug.cgi?id=855165
https://bugzilla.mozilla.org/show_bug.cgi?id=828923
...

Peter, could you help us in order to identify all duplicates? Thanks!
Flags: needinfo?(pdolanjski)
Flags: needinfo?(jcheng)
Flags: needinfo?(fbsc)
Attachment #739051 - Flags: review?(felash) → review?(fbsc)
Comment on attachment 739051 [details] [review]
PR to fix case when SMS app attempts to show deleted message from new message notification

This is a UI workaround, but the point here it's that we have other workarounds as well with the notification stuff... We need changes in Gecko, and we need because we have this bug duplicated several times, what it means that it's important to fix. This it's easy to fix if notification API offer a method 'delete'. I would like to get this functionality, and having a Gaia patch for using this.
Attachment #739051 - Flags: review?(fbsc) → review-
Borja, I think that in this bug we won't remove the notification (see comment 5).

The new notification API landed in Gecko in Bug 782211 but AFAIK won't be uplifted to the b2g18 branches because it's too complicated.

So in this bug we only want a simple UI workaround.
see also Bug 855165 comment 4: we won't do the proper fix in 1.1.
Vicamo / Steve, mind taking a look at comment 15? Thanks
Flags: needinfo?(vyang)
Flags: needinfo?(schung)
Flags: needinfo?(jcheng)
Like what Julien had said, bug 782211 is already implemented in m-c, just have to take the risk to uplift it to b2g18.
Flags: needinfo?(vyang)
The risk and the time. I believe we have more important things to do.
Hey guys

Attached is a proposed interim solution for bug 837029. Happy to take feedback if it does not work for you or you require further detail. However we should strive to get the notifications communicating with the apps asap at which point this solution will be superseded.

The flow attached here will appear as part of the next wireframe pack release (v7.0) for SMS/MMS user stories, but i wanted to issue it early so UX is no longer blocking your progress.
Flags: needinfo?(aymanmaat)
(In reply to ayman maat :maat from comment #22)
> However
> we should strive to get the notifications communicating with the apps asap
> at which point this solution will be superseded.

I believe this will be done for v1.2, I'd be so happy to do that !

> 
> The flow attached here will appear as part of the next wireframe pack
> release (v7.0) for SMS/MMS user stories, but i wanted to issue it early so
> UX is no longer blocking your progress.

thanks a lot !
Flags: needinfo?(schung)
Flags: needinfo?(pdolanjski)
Satender, if you can update the PR tomorrow I'll be happy to review it.

As you may see on the proposition from Ayman, there is a title + a body so you can't use the normal alert method.

There is a CustomDialog object that you might use but I think we're trying to kill it, so the best way to do it is to add a custom dialog in index.html and to use it to display such alerts. I think we do this already for a custom confirm box.
Assignee: fernando.campo → ssaroha
Julien, as suggested have updated the PR with custom dialog to show both title and message. please review.
Attachment #739051 - Flags: review?(felash)
Comment on attachment 739051 [details] [review]
PR to fix case when SMS app attempts to show deleted message from new message notification

Due to last update from Ayman, reviewing again.
Attachment #739051 - Flags: review- → review?(fbsc)
Hi Satender! I've talked with Ayman. As this is a 'workaround' (I mean, it's because `Notifications API` is not complete), we can live without the title. So please, move forward with the simplest solution (in this case a native `alert`), and ask me to review this patch (Julien, Im gonna steal this review for landing it asap! ;) ). We will have it landed as soon as you create the small patch! Thanks
Attachment #739051 - Flags: review?(felash)
Review done. Please address the comment https://github.com/mozilla-b2g/gaia/pull/9276#discussion_r3997532 and I will review it again. Thanks! Gracias! ;)
Comment on attachment 739051 [details] [review]
PR to fix case when SMS app attempts to show deleted message from new message notification

Please ask to review again when ready. Thanks!
Attachment #739051 - Flags: review?(fbsc)
Comment on attachment 739051 [details] [review]
PR to fix case when SMS app attempts to show deleted message from new message notification

borja is reviewing this currently
Attachment #739051 - Flags: review?(fbsc)
Comment on attachment 739051 [details] [review]
PR to fix case when SMS app attempts to show deleted message from new message notification

oops sorry I just saw that borja removed the flag himself, sorry.
Attachment #739051 - Flags: review?(fbsc)
Comment on attachment 739051 [details] [review]
PR to fix case when SMS app attempts to show deleted message from new message notification

updating the bugzilla, to indicate the latest status. its ready for review after incorporating the suggestions from Borja. for details see conversation in PR.
Attachment #739051 - Flags: review?(fbsc)
will need a copy review for the new string (but I don't know how to request that)
Attachment #739051 - Flags: review?(fbsc) → review+
John, can you please assist with the v1-train uplift?
Flags: needinfo?(jhford)
I still think we should ask a copy review for the new string which does not feel good to me, and I am quite upset that you merged without even commenting my comment 33.
Maat, could you check this new string please ? Are you the good one to ask ?
Flags: needinfo?(aymanmaat)
Julien, I've merged due to l10n gave Satender the feedback+, so I thought that it was the right one. Sorry for the missunderstanding, because I thought that your comment was older than feedback+ from l10n team. Let me know if we have to change this and I will file a bug for making it happens.
I can't reconstruct what I actually commented with the pull request, sadly. Without precise history, I'd not take a comment that's 10 days old over a comment that's a day old.

ad-hoc comments from a non-native speaker:

"sorry" might be too personal, not sure. If it's OK, it probably need to be delimited by a ',', like, "sorry, we failed". Also I miss a trailing '.'.
Borja> Pike gave a technical feedback+, but not a string copy review feedback+. As he said already, he's not goving string copy reviews because he's not a native english speaker (and we're not either, that's why we need native english speaker review this).

Pike, who should we ask ui-review to ? Our UX designer Ayman is not a native english speaker either...
Whiteboard: [QE1][TD-10058] → [QE1][TD-10058][ui-review}
(In reply to Julien Wajsberg [:julienw] from comment #37)
> Maat, could you check this new string please ? Are you the good one to ask ?

"Sorry but the message you are trying to access has already been deleted" seems fine to me. but i am sure a copywriter could do a better job...
Flags: needinfo?(aymanmaat)
Using the new :fxosux alias for the first time: We'd love copy feedback and English grammar on

> Sorry but the message you are trying to access has already been deleted
Flags: needinfo?(firefoxos-ux-bugzilla)
HI Tayler,
can you have a look at the proposed text by Ayman "Sorry but the message you are trying to access has already been deleted"? Really appreciated your feedback :)
Flags: needinfo?(tyler.altes)
(In reply to Maria Angeles Oteo:oteo from comment #43)
> HI Tayler,
> can you have a look at the proposed text by Ayman "Sorry but the message you
> are trying to access has already been deleted"? Really appreciated your
> feedback :)

I would just simplify the text a bit: "This message has already been deleted."

Also, I would remove the title. 
Based on comments above, it seems that the title isn't part of the standard module and I don't see the need to have a title on this message. If there isn't a specific reason to keep it, I would remove it and just display the body text.
Flags: needinfo?(tyler.altes)
Tyler> about removing the title, that was the implementation actually. We were looking for a copy review for the body only. Thanks !

Maria, Pike, Ayman, Borja, satender, is that good for you ?
If the title is removed for me it's OK
I mean, is the new text good for you ?

satender, could you prepare a new patch for this text change please ?
(In reply to Julien Wajsberg [:julienw] from comment #47)
> I mean, is the new text good for you ?
> 
> satender, could you prepare a new patch for this text change please ?

New text is fine by me. thanks Tyler.
Julien,
here is the PR for text change. https://github.com/mozilla-b2g/gaia/pull/9575.
Attachment #746221 - Flags: review?(fbsc)
Comment on attachment 746221 [details] [review]
PR for text change after copy review

r=me

thanks, I'm merging it
Attachment #746221 - Flags: review?(fbsc) → review+
master: c79b70526e676c880b4bbd293b97bb20a0e9eeb2


so John, there are 2 commits to uplift here :
1: 23f0b698f96affd9242382178202bedb14bc91ad
2: c79b70526e676c880b4bbd293b97bb20a0e9eeb2

Thanks !
Flags: needinfo?(firefoxos-ux-bugzilla)
Priority: P2 → P1
(In reply to Ryan VanderMeulen [:RyanVM] from comment #35)
> John, can you please assist with the v1-train uplift?

John, ping :)
Gareth, as you're the one doing uplifts this week ;)
Flags: needinfo?(gaye)
v1-train:
1: 35f497c9c8705953d896d2fdea6cb0da7e9a69ae
2: 95f5272b10c68dd2f7786ead0fd06bdb6f2b9b46
Flags: needinfo?(jhford)
Flags: needinfo?(gaye)
Thanks Julien! I'll get to this later this morning.
Flags: in-moztrap?
Flags: in-moztrap? → in-moztrap?(ahubenya)
Flags: in-moztrap?(ahubenya) → in-moztrap+
Issue is verified as fixed. 
"This message has already been deleted" information message is displayed when user attempts to open already deleted sms from the notifications.

Unagi Build ID: 20130618070211
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/a199b1109860
Gaia: 3e9090894daaa1c7f894a1dcc1026b21f889eadc
Platform Version: 18.0
Status: RESOLVED → VERIFIED
Attachment mime type: text/plain → text/x-github-pull-request
Attachment mime type: text/plain → text/x-github-pull-request
You need to log in before you can comment on or make changes to this bug.