Closed Bug 1013295 Opened 9 years ago Closed 9 years ago

[Messages] Migrated messages don't have a proper sentTimestamp

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:1.4+, b2g-v1.3 wontfix, b2g-v1.3T wontfix, b2g-v1.4 fixed, b2g-v2.0 fixed)

RESOLVED FIXED
2.0 S3 (6june)
blocking-b2g 1.4+
Tracking Status
b2g-v1.3 --- wontfix
b2g-v1.3T --- wontfix
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: julienw, Assigned: azasypkin)

References

Details

(Whiteboard: [p=1])

Attachments

(2 files)

I just noticed that messages coming from earlier db don't have a sentTimestamp, and that in Gaia we display it as "1st January 1970" which is obviously wrong.

Before fixing it in Gaia, I'd like to know if it's expected that migrated messages don't have a proper sentTimestamp property, so NI Bevis on this.

This is a 1.4 feature (bug 901457 and bug 961574).
Flags: needinfo?(btseng)
1.4+
blocking-b2g: 1.4? → 1.4+
I'm not sure this qualifies as regression because the feature is new to 1.4.
(In reply to Julien Wajsberg [:julienw] from comment #2)
> I'm not sure this qualifies as regression because the feature is new to 1.4.

How is this a new feature?
Displaying the sent timestamps is new to 1.4 (bug 901457 and bug 961574).

It's different to displaying the received timestamp which works fine.
The NI to Bevis is essentially: did we have the information stored in previous FxOS versions, and we just needed to expose it (and this doesn't work for older messages), or was the information just not stored (in that case we only need to hide the information in Gaia).
Keywords: regression
Ok - so that wouldn't be a regression or data loss - it's just a bad migration issue.
Keywords: dataloss
(In reply to Julien Wajsberg [:julienw] from comment #0)
> I just noticed that messages coming from earlier db don't have a
> sentTimestamp, and that in Gaia we display it as "1st January 1970" which is
> obviously wrong.
> 
> Before fixing it in Gaia, I'd like to know if it's expected that migrated
> messages don't have a proper sentTimestamp property, so NI Bevis on this.
> 
> This is a 1.4 feature (bug 901457 and bug 961574).

It's intended in the fix of bug 901457 [1], because
there is no valid sentTimestamp recovered from the old records in DB.

[1] http://hg.mozilla.org/mozilla-central/annotate/16a36f5bb9a8/dom/mobilemessage/src/gonk/MobileMessageDB.jsm#l1285
Flags: needinfo?(btseng)
So it's a Gaia only fix: we need to hide the information if the sentTimestamp is 0.

Thanks a lot!
Will look into it.
Assignee: nobody → azasypkin
Status: NEW → ASSIGNED
OS: Linux → Gonk (Firefox OS)
Hardware: x86_64 → ARM
Patch that hides sent and received timestamp labels if corresponding timestamp isn't valid.
Attachment #8427549 - Flags: review?(felash)
Comment on attachment 8427549 [details] [review]
GitHub pull request URL (v1.4)

redirect review to Steve :)
Attachment #8427549 - Flags: review?(felash) → review?(schung)
Comment on attachment 8427549 [details] [review]
GitHub pull request URL (v1.4)

Left a comment on github about the necessity of checking timestampt property. Maybe we could make the code clearner without it.
[1] https://github.com/mozilla-b2g/gaia/pull/19556/files#discussion_r13035447
Attachment #8427549 - Flags: review?(schung)
Blocks: sms-sprint-2
Whiteboard: [p=1]
Target Milestone: --- → 2.0 S3 (6june)
Here is the patch rebased on master branch.
Attachment #8429076 - Flags: review?(schung)
Comment on attachment 8427549 [details] [review]
GitHub pull request URL (v1.4)

(In reply to Steve Chung [:steveck] from comment #12)
> Comment on attachment 8427549 [details] [review]
> GitHub pull request URL (v1.4)
> 
> Left a comment on github about the necessity of checking timestampt
> property. Maybe we could make the code clearner without it.
> [1] https://github.com/mozilla-b2g/gaia/pull/19556/files#discussion_r13035447

Thanks for review! Comments are addressed.
Attachment #8427549 - Flags: review?(schung)
Comment on attachment 8427549 [details] [review]
GitHub pull request URL (v1.4)

r=me with some suggestion, please rebase the patch and we are ready to go!
Attachment #8427549 - Flags: review?(schung) → review+
Comment on attachment 8429076 [details] [review]
GitHub pull request URL

r=me and suggestion is same for the v1.4 patch, thanks for the nice job :)
Attachment #8429076 - Flags: review?(schung) → review+
in master: ae083fa087c0e9c1f84558e1c2d7e77eec29b3fc
(In reply to Steve Chung [:steveck] from comment #15)
> Comment on attachment 8427549 [details] [review]
> GitHub pull request URL (v1.4)
> 
> r=me with some suggestion, please rebase the patch and we are ready to go!

Please flag checkin-needed when commits squashed properly, thanks.
Flags: needinfo?(azasypkin)
Note to sheriffs: check-in is needed for "GitHub pull request URL (v1.4)" patch only.

Thanks!
Flags: needinfo?(azasypkin)
Keywords: checkin-needed
Failing on Travis.
Keywords: checkin-needed
Also, FYI, you don't need to set checkin-needed on 1.4+ uplifts :)
Bug 1016852 is filed for the failing issue in Travis, it's unrelated to this, so I'm gonna merge :)
v1.4: 5527784b263755f55b620441fc95a66496fa18cf
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.