Julien Wajsberg [:julienw] Reporter
	Description • 10 years ago

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

	Preeti Raghunath(:Preeti)
	Comment 1 • 10 years ago

1.4+

	Julien Wajsberg [:julienw] Reporter
	Comment 2 • 10 years ago

I'm not sure this qualifies as regression because the feature is new to 1.4.

	Jason Smith [:jsmith]
	Comment 3 • 10 years ago

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

	Julien Wajsberg [:julienw] Reporter
	Comment 4 • 10 years ago

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.

	Julien Wajsberg [:julienw] Reporter
	Comment 5 • 10 years ago

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

	Jason Smith [:jsmith]
	Comment 6 • 10 years ago

Ok - so that wouldn't be a regression or data loss - it's just a bad migration issue.

	Bevis Tseng[:bevis][:btseng](Exited)
	Comment 7 • 10 years ago

(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

	Julien Wajsberg [:julienw] Reporter
	Comment 8 • 10 years ago

So it's a Gaia only fix: we need to hide the information if the sentTimestamp is 0.

Thanks a lot!

	Oleg Zasypkin [:azasypkin] Assignee
	Comment 9 • 10 years ago

Will look into it.

	Oleg Zasypkin [:azasypkin] Assignee
	Comment 10 • 10 years ago

Patch that hides sent and received timestamp labels if corresponding timestamp isn't valid.

	Julien Wajsberg [:julienw] Reporter
	Comment 11 • 10 years ago

Comment on attachment 8427549 [details] [review]
GitHub pull request URL (v1.4)

redirect review to Steve :)

	Steve Chung [:steveck]
	Comment 12 • 10 years ago

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

	Oleg Zasypkin [:azasypkin] Assignee
	Comment 13 • 10 years ago

Here is the patch rebased on master branch.

	Oleg Zasypkin [:azasypkin] Assignee
	Comment 14 • 10 years ago

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.

	Steve Chung [:steveck]
	Comment 15 • 10 years ago

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!

	Steve Chung [:steveck]
	Comment 16 • 10 years ago

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

	Steve Chung [:steveck]
	Comment 17 • 10 years ago

in master: ae083fa087c0e9c1f84558e1c2d7e77eec29b3fc

	Steve Chung [:steveck]
	Comment 18 • 10 years ago

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

	Oleg Zasypkin [:azasypkin] Assignee
	Comment 19 • 10 years ago

Note to sheriffs: check-in is needed for "GitHub pull request URL (v1.4)" patch only.

Thanks!

	Ryan VanderMeulen [:RyanVM]
	Comment 20 • 10 years ago

Failing on Travis.

	Ryan VanderMeulen [:RyanVM]
	Comment 21 • 10 years ago

Also, FYI, you don't need to set checkin-needed on 1.4+ uplifts :)

	Julien Wajsberg [:julienw] Reporter
	Comment 22 • 10 years ago

Bug 1016852 is filed for the failing issue in Travis, it's unrelated to this, so I'm gonna merge :)

	Julien Wajsberg [:julienw] Reporter
	Comment 23 • 10 years ago

v1.4: 5527784b263755f55b620441fc95a66496fa18cf