oppia-android icon indicating copy to clipboard operation
oppia-android copied to clipboard

Investigate second/millisecond conversion check for greeting timestamp

Open BenHenning opened this issue 4 years ago • 5 comments

After #3795, TextViewBindingAdapters performs a check on whether the last visited timestamp is in milliseconds or seconds. We ought to just use milliseconds & never need this conversion, but investigation is needed to better understand where this check came from & if it could ever be triggered.

BenHenning avatar Sep 25 '21 08:09 BenHenning

@BenHenning fun ensureTimestampIsInMilliseconds convert the given time into ms only if lastVisitedTimestamp < 1000000000000L. While working on #3846 I realized that the time which we are passing is the function setProfileLastVisitedText is already taken in ms/s in all the test cases written for TextViewBindingAdapters and we are using FakeOppiaClock and setting fake time mode as fixed withTIMESTAMP = 1556094120000 → Time: Wed Apr 24 2019 08:22:00 which is always going to be greater than 1000000000000L even if we subtract some days/months/years in ms to get last active time of user.

image

See conversion if user was last active 18 years ago then this condition will be satisfied and user will get wrong output.

image

The argument which ensureTimestampIsInMilliseconds takes is always in ms, even if it is < 1000000000000L, but if this condition is satisfied then we are basically changing the input time to 1000*inputTime which is not correct instead we should remove this check.

So, my verdict is that this function is not correct and if input time is ensured to be in ms whenever we are using fun setProfileLastVisitedText because ensureTimestampIsInMilliseconds will alter the input even if we don't want to. Also, this is not a correct way to check if the input time is in ms.

I hope this makes some sense.

But If we look for real life scenarios we would never encounter these conditions.

rishidyno avatar Feb 11 '22 12:02 rishidyno

I think you're right that it isn't needed @rishidyno, however before we remove it could you first look through the blame history for it to see if there's any context on why this is needed? You may need to go all the way back to the PR that introduced it and that PR's description and/or conversation threads to get this context.

BenHenning avatar Feb 19 '22 02:02 BenHenning

@BenHenning this fun was introduced of one of your PR #3795. I tried to find any context on why this fun was introduced, but could not find any. Either way, as I explained earlier, this fun will just mess up things if we wrote a hypothetical test for time 18 years ago, which I did try and as expected it failed. I recommend removing it as it's unnecessary as the argument of this function is already in Milliseconds.

rishidyno avatar Feb 26 '22 17:02 rishidyno

I have just created a pr. If the final verdict is to remove the fun, we can just simply merge this pr. Thanks!

rishidyno avatar Feb 28 '22 15:02 rishidyno

@rishidyno it actually wasn't introduced in that PR, the utility was just moved (original file: https://github.com/oppia/oppia-android/blob/713584ae1d7d5a9edce6c9838f9c0301c96febbd/utility/src/main/java/org/oppia/android/util/system/OppiaDateTimeFormatter.kt). That was originally located at https://github.com/oppia/oppia-android/blob/44cf7871feeaeb16cbec4584caaa0f74fb17af12/utility/src/main/java/org/oppia/util/system/OppiaDateTimeFormatter.kt before we moved everything. The utility was introduced in #958, but it interestingly moved this logic out of the adapters class (so it's come full loop).

It seems like the origin of the code is #580. You should look through that PR and its conversations to see if you can figure out the origin of that constant.

BenHenning avatar Mar 01 '22 04:03 BenHenning