Investigate second/millisecond conversion check for greeting timestamp

[BenHenning]

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.

[rishidyno]

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

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

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.

[BenHenning]

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.

[rishidyno]

@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]

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

[BenHenning]

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

[adhiamboperes]

@dattasneha, you may be interested in this issue. PTAL.

[dattasneha]

@dattasneha, you may be interested in this issue. PTAL.

Sure @adhiamboperes, I would like to work on this. After going through the issue thread, I believe simply removing the function ensureTimestampIsInMilliseconds would resolve the issue.

[adhiamboperes]

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

This is a redundant check. The timestamp that is bound from the adapter comes from the profile proto, and is set to ms in the ProfileManagementController. It is safe to remove the check.