Closed
Bug 837029
Opened 12 years ago
Closed 11 years ago
[SMS] Messages app attempts to show deleted SMS if new message notification still exists
Categories
(Core :: DOM: Device Interfaces, defect, P1)
Tracking
()
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
Comment 1•12 years ago
|
||
This is related with Gecko, due to we have no way for managing previously created notifications. Updating the bug...
Updated•12 years ago
|
Component: Gaia::SMS → DOM: Device Interfaces
Product: Boot2Gecko → Core
QA Contact: mbarone976
Updated•12 years ago
|
tracking-b2g18:
--- → +
Comment 3•12 years ago
|
||
This issue has been identified as a QE1 blocker -- nominating for Leo.
blocking-b2g: --- → leo?
Whiteboard: [QE1]
Comment 4•12 years ago
|
||
What is the detailed UX design here ?
Should we display an information message to the user in this case ?
Flags: needinfo?(aymanmaat)
Updated•12 years ago
|
Target Milestone: --- → Leo QE1 (5may)
Comment 5•12 years ago
|
||
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+
Updated•12 years ago
|
Assignee: nobody → fernando.campo
Comment 6•12 years ago
|
||
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.
Comment 8•12 years ago
|
||
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
Assignee | ||
Comment 10•12 years ago
|
||
Attachment #739051 -
Flags: review?(felash)
Comment 11•12 years ago
|
||
borja, feel free to steal that review if you want it, I'm still pretty busy with tef+ stuff.
Flags: needinfo?(fbsc)
Comment 12•12 years ago
|
||
Comment on attachment 739051 [details]
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 13•12 years ago
|
||
Comment on attachment 739051 [details]
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+
Comment 14•12 years ago
|
||
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
Updated•12 years ago
|
Priority: -- → P2
Comment 15•12 years ago
|
||
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)
Updated•12 years ago
|
Attachment #739051 -
Flags: review?(felash) → review?(fbsc)
Comment 16•12 years ago
|
||
Comment on attachment 739051 [details]
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-
Comment 17•12 years ago
|
||
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.
Comment 18•12 years ago
|
||
see also Bug 855165 comment 4: we won't do the proper fix in 1.1.
Comment 19•11 years ago
|
||
Vicamo / Steve, mind taking a look at comment 15? Thanks
Flags: needinfo?(vyang)
Flags: needinfo?(schung)
Flags: needinfo?(jcheng)
Comment 20•11 years ago
|
||
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)
Comment 21•11 years ago
|
||
The risk and the time. I believe we have more important things to do.
Comment 22•11 years ago
|
||
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)
Comment 23•11 years ago
|
||
(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)
Updated•11 years ago
|
Flags: needinfo?(pdolanjski)
Comment 24•11 years ago
|
||
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
Assignee | ||
Comment 25•11 years ago
|
||
Julien, as suggested have updated the PR with custom dialog to show both title and message. please review.
Attachment #739051 -
Flags: review?(felash)
Comment 26•11 years ago
|
||
Comment on attachment 739051 [details]
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)
Comment 27•11 years ago
|
||
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
Updated•11 years ago
|
Attachment #739051 -
Flags: review?(felash)
Comment 28•11 years ago
|
||
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 29•11 years ago
|
||
Comment on attachment 739051 [details]
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 30•11 years ago
|
||
Comment on attachment 739051 [details]
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 31•11 years ago
|
||
Comment on attachment 739051 [details]
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)
Assignee | ||
Comment 32•11 years ago
|
||
Comment on attachment 739051 [details]
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)
Comment 33•11 years ago
|
||
will need a copy review for the new string (but I don't know how to request that)
Updated•11 years ago
|
Attachment #739051 -
Flags: review?(fbsc) → review+
Comment 34•11 years ago
|
||
https://github.com/mozilla-b2g/gaia/commit/23f0b698f96affd9242382178202bedb14bc91ad
https://github.com/ssaroha/gaia/commit/0d06538f11792582aaea0651d2e4b773dedc174e
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Comment 35•11 years ago
|
||
John, can you please assist with the v1-train uplift?
Flags: needinfo?(jhford)
Comment 36•11 years ago
|
||
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.
Comment 37•11 years ago
|
||
Maat, could you check this new string please ? Are you the good one to ask ?
Flags: needinfo?(aymanmaat)
Comment 38•11 years ago
|
||
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.
Comment 39•11 years ago
|
||
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 '.'.
Comment 40•11 years ago
|
||
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}
Comment 41•11 years ago
|
||
(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)
Comment 42•11 years ago
|
||
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)
Comment 43•11 years ago
|
||
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)
Updated•11 years ago
|
tracking-b2g18:
+ → ---
Comment 44•11 years ago
|
||
(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)
Comment 45•11 years ago
|
||
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 ?
Comment 46•11 years ago
|
||
If the title is removed for me it's OK
Comment 47•11 years ago
|
||
I mean, is the new text good for you ?
satender, could you prepare a new patch for this text change please ?
Comment 48•11 years ago
|
||
(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.
Assignee | ||
Comment 49•11 years ago
|
||
Julien,
here is the PR for text change. https://github.com/mozilla-b2g/gaia/pull/9575.
Assignee | ||
Comment 50•11 years ago
|
||
Attachment #746221 -
Flags: review?(fbsc)
Comment 51•11 years ago
|
||
Comment on attachment 746221 [details]
PR for text change after copy review
r=me
thanks, I'm merging it
Attachment #746221 -
Flags: review?(fbsc) → review+
Comment 52•11 years ago
|
||
master: c79b70526e676c880b4bbd293b97bb20a0e9eeb2
so John, there are 2 commits to uplift here :
1: 23f0b698f96affd9242382178202bedb14bc91ad
2: c79b70526e676c880b4bbd293b97bb20a0e9eeb2
Thanks !
Flags: needinfo?(firefoxos-ux-bugzilla)
Updated•11 years ago
|
status-b2g18:
--- → affected
Comment 53•11 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM] from comment #35)
> John, can you please assist with the v1-train uplift?
John, ping :)
Comment 54•11 years ago
|
||
Gareth, as you're the one doing uplifts this week ;)
Flags: needinfo?(gaye)
Comment 55•11 years ago
|
||
v1-train:
1: 35f497c9c8705953d896d2fdea6cb0da7e9a69ae
2: 95f5272b10c68dd2f7786ead0fd06bdb6f2b9b46
Comment 56•11 years ago
|
||
Thanks Julien! I'll get to this later this morning.
Updated•11 years ago
|
Flags: in-moztrap?
Updated•11 years ago
|
Flags: in-moztrap? → in-moztrap?(ahubenya)
Updated•11 years ago
|
Flags: in-moztrap?(ahubenya) → in-moztrap+
Comment 57•11 years ago
|
||
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
Updated•11 years ago
|
Attachment mime type: text/plain → text/x-github-pull-request
Updated•11 years ago
|
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.
Description
•