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

Status: VERIFIED FIXED

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

Description

[Flaburgan]

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.

Comment 1

[Flaburgan]

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.

Comment 2

[Preeti Raghunath(:Preeti)]

Naoki

Please check if this reproduce still

Comment 3

[Peter Bylenga [:PBylenga]]

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

Comment 4

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

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

Comment 5

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Er sorry.  I meant to say that it does NOT occur on 1.2  It's a regression on 1.3

Comment 6

[Julien Wajsberg [:julienw]]

Can we know whether this happens in master too?

I'd bet on bug 929919 as the regressing bug.

Comment 7

[Peter Bylenga [:PBylenga]]

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

Comment 8

[Julien Wajsberg [:julienw]]

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

Comment 9

[Joe Cheng [:jcheng] (please needinfo)]

triage: 1.3+ regression

Comment 10

[Peter Bylenga [:PBylenga]]

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

Comment 11

[Julien Wajsberg [:julienw]]

bug 929919 is in that range.

Comment 12

[Preeti Raghunath(:Preeti)]

Kaze,

Next steps, please.

Comment 13

[Fabien Cazenave [:kaze]]

I’m on it.

Comment 14

[Fabien Cazenave [:kaze]]

Attachment: link to pull request

Comment 15

[Fabien Cazenave [:kaze]]

Attachment: 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.

Comment 16

[Julien Wajsberg [:julienw]]

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?

Comment 17

[Steve Chung [:steveck]]

Ok I'll take a look first.

Comment 18

[Borja Salguero [:borjasalguero]]

As Steve is taking a look, removing ni.

Comment 19

[Steve Chung [:steveck]]

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.

Comment 20

[Fabien Cazenave [:kaze]]

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.

Comment 21

[Rick Waldron [:rwaldron]]

Comment on attachment 8375906 [details] [review]
link to pull request

Add tests for this change and then r? either schung or me, thanks

Comment 22

[Steve Chung [:steveck]]

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 23

[Fabien Cazenave [:kaze]]

Comment on attachment 8375906 [details] [review]
link to pull request

Thanks for the comments, here’s an updated patch (with a bonus fix).

Comment 24

[Steve Chung [:steveck]]

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

Comment 25

[Fabien Cazenave [:kaze]]

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.

Comment 26

[Julien Wajsberg [:julienw]]

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?

Comment 27

[Fabien Cazenave [:kaze]]

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

Comment 28

[Julien Wajsberg [:julienw]]

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.

Comment 29

[Fabien Cazenave [:kaze]]

Thanks for the pointer. Looking.

Comment 30

[Steve Chung [:steveck]]

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

Comment 31

[Fabien Cazenave [:kaze]]

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

[Fabien Cazenave [:kaze]]

Comment on attachment 8375906 [details] [review]
link to pull request

PR updated. Julien, can you please have a look?

Comment 33

[Julien Wajsberg [:julienw]]

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.

Comment 34

[Fabien Cazenave [:kaze]]

Merged on master:
https://github.com/mozilla-b2g/gaia/commit/988c320bccfa7f873dc2ef3e7c34a11a9f4ad409

Comment 35

[Ryan VanderMeulen [:RyanVM]]

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

Comment 36

[Fabien Cazenave [:kaze]]

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

Comment 37

[Flaburgan]

Thanks for the great work guys!

Comment 38

[Ryan VanderMeulen [:RyanVM]]

This doesn't cherry-pick cleanly to v1.3. Please rebase.

Comment 39

[Fabien Cazenave [:kaze]]

Attachment: link to pull request (1.3 branch)

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.

Comment 40

[Fabien Cazenave [: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!

Comment 41

[Julien Wajsberg [:julienw]]

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

Comment 42

[Ryan VanderMeulen [:RyanVM]]

Thanks :)

v1.3: a43904d9646109b48836d62f7aa51e499fbf4b2e

Comment 43

[Loli (:lolimartinezcr)]

Tested 02/19/2014 and working
1.3
Gecko 0b4f0a8
Gaia a43904d

master
Gecko 47d3bb4
Gaia 6e71ab4