Closed Bug 968815 Opened 7 years ago Closed 7 years ago

[SMS] Delete a SMS doesn't refresh the main list so it is outdated

Categories

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

defect
Not set
normal

Tracking

(blocking-b2g:1.3+, b2g-v1.2 unaffected, b2g-v1.3 fixed, b2g-v1.3T fixed, b2g-v1.4 fixed)

VERIFIED FIXED
1.4 S1 (14feb)
blocking-b2g 1.3+
Tracking Status
b2g-v1.2 --- unaffected
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed

People

(Reporter: flaburgan, Assigned: kaze)

References

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:26.0) Gecko/20100101 Firefox/26.0 (Beta/Release)
Build ID: 20131206145143

Steps to reproduce:

Open the SMS application, you're now on the listing conversation view
Open a conversation
Delete the last message of the conversation
Go back to the listing conversation view


Actual results:

The conversation still displays the SMS deleted as the last SMS of the conversation


Expected results:

The SMS deleted should not be displayed.



Btw, if the SMS application is killed and then launched again, the SMS deleted disappears.
I have the issue on a Keon with the last 1.3.0.0-prerelease version provided by Geeksphone. It looks like this is a regression, the bug is not present in the actual 1.2.0.0-prerelease version by Geeksphone.

One of the consequence of the bug is that the conversations are not ordered correctly in the list.
blocking-b2g: --- → 1.3?
Assignee: nobody → kaze
Naoki

Please check if this reproduce still
Flags: needinfo?(nhirata.bugzilla)
Keywords: qawanted
I can reproduce the actual results given the STRs in Comment 0 on today's Buri v1.3.

v1.3 Environmental Variables:
Device: Buri v1.3 MOZ
BuildID: 20140206004002
Gaia: 467ef8c9145d9a57d35b0619db541d23b522b958
Gecko: a1fa925c40c2
Version: 28.0
Firmware Version: v1.2-device.cfg
Keywords: qawanted
I confirmed that this still occurs on : 1.2 as well.  looks like it's a regression on 1.3

Gaia      539a25e1887b902b8b25038c547048e691bd97f6
Gecko     https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/1592ce49929c
BuildID   20140205004001
Version   26.0
ro.build.version.incremental=324
ro.build.date=Thu Dec 19 14:04:55 CST 2013
Buri
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(nhirata.bugzilla)
Keywords: regression
Er sorry.  I meant to say that it does NOT occur on 1.2  It's a regression on 1.3
Can we know whether this happens in master too?

I'd bet on bug 929919 as the regressing bug.
Keywords: qawanted
Issue reproduces identically to my findings in Comment 3.  Going back to Messages list view after deleting the newest SMS in a conversation still shows the deleted SMS as the last SMS of the conversation.

v1.4 Environmental Variables:
Device: Buri v1.4 MOZ
BuildID: 20140207040203
Gaia: 3fc26ae786e3869a7ef1e23afc9807ac1b4741f2
Gecko: d05c721ea1b0
Version: 30.0a1
Firmware Version: v1.2-device.cfg
Keywords: qawanted
QA Contact: pbylenga
Kaze, I had a look quickly, and strangely, we're supposed to update the thread at [1], so probably this piece of code does not work properly. Maybe the lastMessageId is not right, or something like this. You'll need to debug using the app manager or logs.

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/js/thread_ui.js#L1750
triage: 1.3+ regression
blocking-b2g: 1.3? → 1.3+
The regression window is 11-27-2013 to 11-28-2013.


First build issue reproduces on:
v1.3 Environmental Variables:
Device: Buri v1.3 MOZ
BuildID: 20131128040201
Gaia: 0d57ec2801ae125ec855a19cf956ab118660d694
Gecko: a5e7f611546f
Version: 28.0a1
Firmware Version: v1.2-device.cfg

Last build issue did not reproduce on:
v1.3 Environmental Variables:
Device: Buri v1.3 MOZ
BuildID: 20131127040203
Gaia: d4b9a3d271f0451b4d903a03c2b931b8cc092041
Gecko: 6ecf0c4dfcbe
Version: 28.0a1
Firmware Version: v1.2-device.cfg
bug 929919 is in that range.
Blocks: 929919
Kaze,

Next steps, please.
Flags: needinfo?(kaze)
I’m on it.
Flags: needinfo?(kaze)
Attached file link to pull request (obsolete) —
Attached file link to pull request
Note: an optimization would be to avoid redrawing the thread when the last message has not been deleted. It’s probably too late to do it in this bug (shame on me), but I guess it could be a follow-up.
Attachment #8375541 - Attachment is obsolete: true
Attachment #8375906 - Flags: review?(felash)
Comment on attachment 8375906 [details] [review]
link to pull request

Borja, Steve, can one of you steal this review today as I'm in PTO and it's a 1.3+ bug?
Flags: needinfo?(schung)
Flags: needinfo?(borja.bugzilla)
Ok I'll take a look first.
Flags: needinfo?(schung)
Attachment #8375906 - Flags: review?(felash) → review?(schung)
As Steve is taking a look, removing ni.
Flags: needinfo?(borja.bugzilla)
Comment on attachment 8375906 [details] [review]
link to pull request

I'm ok with optimizing the thread updating in the future, but this patch will trigger another bug in message deleting. Find a conversation contains more than one datetime section header(like TODAY/YESTERDAY/...) and remove message. You will see wrong thread updated in thread list view.

The problem would be https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/js/thread_ui.js#L1744. This selector will only selects the last li in 'first ul', so please make sure we are selecting the last li in the 'last ul' before we actually decoupling the message data and message view in the future.
Attachment #8375906 - Flags: review?(schung)
Comment on attachment 8375906 [details] [review]
link to pull request

Thanks for the sharp pointer Steve, that helped a lot — and that’s the part I missed for the optimization I mentioned in comment #15.

With this updated patch, the thread UI is not updated any more when the user deletes a message that is not the latest one.
Attachment #8375906 - Flags: review?(schung)
Comment on attachment 8375906 [details] [review]
link to pull request

Add tests for this change and then r? either schung or me, thanks
Attachment #8375906 - Flags: review?(schung) → review-
Comment on attachment 8375906 [details] [review]
link to pull request

Hi kaze, I left some comments on github and most of them are minor issue. Hope that we can land this today :)
Comment on attachment 8375906 [details] [review]
link to pull request

Thanks for the comments, here’s an updated patch (with a bonus fix).
Attachment #8375906 - Flags: review- → review?(schung)
(In reply to Fabien Cazenave [:kaze] from comment #23)
> Comment on attachment 8375906 [details] [review]
> link to pull request
> 
> Thanks for the comments, here’s an updated patch (with a bonus fix).

Sorry I don't quite understand why we need to update to 'read' status after deleting. Could you give an example why we need this statement?
To update a thread in the UI, we just delete it and re-create it, which makes it appear as `unread' by default.

As the thread has to be read before messages can be deleted, this patch forces the `read' status.
I don't understand this... I think we can totally append a thread in an "read" status, I mean, we do this when starting the app, right?
(In reply to Julien Wajsberg [:julienw] from comment #26)
> I think we can totally append a thread in an "read" status

Well, that’s the only way I found to do it without spaghettifying the code. :-/ I might have missed something, though.

The ThreadListUI.appendThread() method takes only one `thread' parameter, and setting thread.read to false before calling appendThread() does not help: the new thread still appears as unread in the UI.

The good thing is that using ThreadListUI.mark('read') make it easy to test.
It's very easy to test by checking the classList anyway ;)

I think we use "unreadCount" to know whether the thread should be read or unread. The "read" property does not exist on a thread. We create a "false" thread from a message in the "Threads.fromMessage" method, so you might want to check there.
Thanks for the pointer. Looking.
Attachment #8375906 - Flags: review?(felash)
(In reply to Fabien Cazenave [:kaze] from comment #27)
> (In reply to Julien Wajsberg [:julienw] from comment #26)
> > I think we can totally append a thread in an "read" status
> 
> Well, that’s the only way I found to do it without spaghettifying the code.
> :-/ I might have missed something, though.
> 
> The ThreadListUI.appendThread() method takes only one `thread' parameter,
> and setting thread.read to false before calling appendThread() does not
> help: the new thread still appears as unread in the UI.
> 
> The good thing is that using ThreadListUI.mark('read') make it easy to test.

I think the problem might be in Thread.fromMessage... the unreadCount is set to:
unreadCount: (options && !options.read) ? 1 : 0,

and when we are only for deleting message(option = {deleted: true}), unreadCount will become 1 here unfortunately... That's pretty risky to have a option like read: false here, so maybe we could change it to unread: true for thread option? Hope it's not a big change here.
(In reply to Steve Chung [:steveck] from comment #30)
> I think the problem might be in Thread.fromMessage... the unreadCount is set
> to:
> unreadCount: (options && !options.read) ? 1 : 0,
> 

Exactly, that’s what my last commit fixed.
https://github.com/fabi1cazenave/gaia/commit/2afcd03e40363a5272d0a89d4690f99cbe753345

> and when we are only for deleting message(option = {deleted: true}),
> unreadCount will become 1 here unfortunately... That's pretty risky to have
> a option like read: false here, so maybe we could change it to unread: true
> for thread option? Hope it's not a big change here.

Yes, that’s exactly what I first did, see my comment on github, but I went back to `read: false` to minimize the patch.

Reverting to `unread: true` and updating all tests.
Comment on attachment 8375906 [details] [review]
link to pull request

PR updated. Julien, can you please have a look?
Attachment #8375906 - Flags: review?(schung)
Comment on attachment 8375906 [details] [review]
link to pull request

r=me

works fine, but I left one nit in the code and 2 nits in the unit tests.

Please merge with a green travis.
Attachment #8375906 - Flags: review?(felash) → review+
Merged on master:
https://github.com/mozilla-b2g/gaia/commit/988c320bccfa7f873dc2ef3e7c34a11a9f4ad409
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Please request gaia v1.3 approval on these patches as 1.3 blockers no longer have auto-approval for landing.
https://wiki.mozilla.org/Release_Management/B2G_Landing#Landing_Procedure_4
Flags: needinfo?(kaze)
Target Milestone: --- → 1.4 S1 (14feb)
Comment on attachment 8375906 [details] [review]
link to pull request

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): bug 929919
[User impact] if declined: when the last message of a thread is deleted, it still appears on the main Messages panel
[Testing completed]: manual + new unit tests
[Risk to taking this patch] (and alternatives if risky): low risk, no alternatives
[String changes made]: none
Attachment #8375906 - Flags: approval-gaia-v1.3?
Flags: needinfo?(kaze)
Attachment #8375906 - Flags: approval-gaia-v1.3? → approval-gaia-v1.3?(fabrice)
Thanks for the great work guys!
Attachment #8375906 - Flags: approval-gaia-v1.3?(fabrice) → approval-gaia-v1.3+
This doesn't cherry-pick cleanly to v1.3. Please rebase.
Flags: needinfo?(kaze)
I can’t merge this myself, this is probably on purpose?

I’ve checked all unit tests locally and tested this patch manually on top of the latest 1.3 pvt-build.
Flags: needinfo?(kaze)
(In reply to Flaburgan from comment #37)
> Thanks for the great work guys!

Thank *you* Antoine for your sharp bug reports. This helps a lot!
Keywords: checkin-needed
(In reply to Fabien Cazenave [:kaze] from comment #39)
> Created attachment 8377884 [details] [review]
> link to pull request (1.3 branch)
> 
> I can’t merge this myself, this is probably on purpose?

Because the tree is closed :)
Thanks :)

v1.3: a43904d9646109b48836d62f7aa51e499fbf4b2e
Tested 02/19/2014 and working
1.3
Gecko 0b4f0a8
Gaia a43904d

master
Gecko 47d3bb4
Gaia 6e71ab4
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.