814163 - Migrate existing data to most-recent table, ensure most-recent items have id

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Dale Harvey (:daleharvey)]

Previous update to the SmsDatabaseService didnt migrate existing data, it also wrote new threads entries without an id

Comment 1

[Dale Harvey (:daleharvey)]

Attachment: Populate most-recent table with existing sms data

Comment 2

[Gregor Wagner [:gwagner]]

Comment on attachment 684164 [details] [diff] [review]
Populate most-recent table with existing sms data

I don't think you can rely on highest message id means youngest message for a thread. You still need to compare the timestamp. I forward to bent since he wrote the mostRecentEntry code.

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 684164 [details] [diff] [review]
Populate most-recent table with existing sms data

Review of attachment 684164 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/sms/src/ril/SmsDatabaseService.js
@@ +335,5 @@
> +      let contact = message.sender || message.receiver;
> +
> +      if (contact in threads) {
> +        let thread = threads[contact];
> +        threads[contact].id = message.id;

Wait, this cursor goes backwards, so the message you're overwriting probably has a newer timestamp, right?

@@ +476,5 @@
>          } else {
>            event.target.source.add({ senderOrReceiver: number,
>                                      timestamp: message.timestamp,
>                                      body: message.body,
> +                                    id: meesage.id,

Typo here ("meesage").

Comment 4

[Dale Harvey (:daleharvey)]

You are right, I think gwagner was right the first time and I would be best just comparing timestamps and not care about what order I traverse, will update tomorrow

Comment 5

[JP Rosevear [:jpr]]

I think this won't affect devices that are not already active (ie production devices)

Please re-nom if we have that wrong.

Comment 6

[Dale Harvey (:daleharvey)]

Attachment: Populate most-recent table with existing sms data

Updated based on review.

In theory this can affect anyone that is currently dogfooding, which would be a blocking+, I dont know enough about the sms app / api usage / deployed versions to know whether dogfooders will actually be affected by it. I am not.

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 685137 [details] [diff] [review]
Populate most-recent table with existing sms data

Review of attachment 685137 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with this one small change.

::: dom/sms/src/ril/SmsDatabaseService.js
@@ +316,5 @@
>                                         { keyPath: "senderOrReceiver" });
>      objectStore.createIndex("timestamp", "timestamp");
>    },
>  
> +  upgradeSchema4: function upgradeSchema4(db, transaction) {

Nit: No need for this to take 'db' argument, you never use it and we don't really have much of a pattern to follow.

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Dale, let's get this in so that Borja can land his SMS changes.

Comment 9

[Dale Harvey (:daleharvey)]

Attachment: Bug 814163 - Populate most-recent table with existing sms data

Nit addressed.

Comment 10

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 686476 [details] [diff] [review]
Bug 814163 - Populate most-recent table with existing sms data

https://hg.mozilla.org/integration/mozilla-inbound/rev/40d061793988

Comment 11

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 686476 [details] [diff] [review]
Bug 814163 - Populate most-recent table with existing sms data

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 809661
User impact if declined: Dogfood devices will soon display an empty thread list in the SMS app.
Testing completed (on m-c, etc.): Dogfood
Risk to taking this patch (and alternatives if risky): Must be done if users have existing data
String or UUID changes made by this patch: None

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/40d061793988

Comment 13

[Alex Keybl [:akeybl]]

Comment on attachment 686476 [details] [diff] [review]
Bug 814163 - Populate most-recent table with existing sms data

In support of existing pre-release testers. Thanks for taking the time to fix Dale/all. If this causes new regressions for whatever reason, we'll cut/run and move on to other b-b+ bugs instead (we'll eat the data loss).

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e49deed923d8
https://hg.mozilla.org/releases/mozilla-beta/rev/4077b03b8c26

Comment 15

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to Ryan VanderMeulen from comment #14)
> https://hg.mozilla.org/releases/mozilla-aurora/rev/e49deed923d8
> https://hg.mozilla.org/releases/mozilla-beta/rev/4077b03b8c26

Thanks Ryan!