Rk unsw

[atton16]

Features

1. Fix invalid date displayed for processed trips

Apparently, the date stored in tripgj.data.properties.start_local_dt and tripgj.data.properties.end_local_dt use a 1-12 month value while moment needs 0-11 month value. The test was done on December which translated to value 12. Hence moment throw Invalid date. However, this problem does not persist for draft trip so we skip the fix for those trips.

Before:

After:

2. Limit date picker to the day the user register (we use timestamp from the oldest user profile survey record)

Example: I answered on January 1st, 2020

[AmplabJenkins]

Can one of the admins verify this patch?

[shankari]

why do you need to do this? you should not be able to retrieve profiles for other users anyway. You can argue that it doesn't hurt anything, but extra code = extra computation = slower response time.

[shankari]

tripgj.data.properties.start_local_dt and tripgj.data.properties.end_local_dt use a 1-12 month value while moment needs 0-11 month value. The test was done on December which translated to value 12. Hence moment throw Invalid date

Are you sure this is the problem? Because if this were the case, then the month would always be off by one (e.g. January would be 1 in the local_dt, converted to February by moment and displayed as such in the UI), and somebody would have noticed before this. Note that getLocalTimeString is used in the master branch as well.

Based on the screenshots, I believe that this fixes the immediate issue that you found, but I am concerned that if it is not based on a well-formed understanding of the underlying bug, it may lead to other regressions.

I would like to see screenshots showing that:

• without this fix, on your channel, November dates are displayed as December and January dates are displayed as February
• on one of the standard channels (e.g. https://e-mission.eecs.berkeley.edu/#/client_setup?new_client=urap2017emotion&clear_usercache=true&clear_local_storage=true), December dates are not displayed correctly

before I approve the change.

[shankari]

ok, I poked around to understand #615 (comment) a bit better. I first confirmed, using the web console, that moment is off by one.

>>> moment({"month": 9, "day": 3, "year": 2016})
Object { _isAMomentObject: true, _i: {…}, _isUTC: false, _pf: {…}, _locale: {…}, _a: (7) […], _d: Date Mon Oct 03 2016 00:00:00 GMT-0700 (Pacific Daylight Time), _isValid: true, _z: null }

But then how does this work on master?! I used the blame feature to go back into the history of getLocalTimeString, and found that, prior to June, we used to manually format the time from the localdate by manipulating the fields directly.

  dh.getLocalTimeString = function(dt) {
      var hr = ((dt.hour > 12))? dt.hour - 12 : dt.hour;
      var post = ((dt.hour >= 12))? " pm" : " am";
      var min = (dt.minute.toString().length == 1)? "0" + dt.minute.toString() : dt.minute.toString();
      return hr + ":" + min + post;
    }

Then, as part of the internationalization (d8e0a27) we used moment to do the formatting, since it would automatically be locale-sensitive.

So why didn't we immediately notice that the month was off? Well, it turns out that we don't display the month! We format the resulting moment as "LT" (which is 08:30 PM or 20:30 in French). So I suspect that the month has been off since June, but we didn't notice it.

However, the date should be displayed as "Invalid" for all of December, so I am still not sure why @PatGendre and @stephhuerre, who are using the FabMob fork with the internationalization changes have not yet noticed this. Maybe they haven't been checking a lot in December because of the holidays?

@PatGendre @stephhuerre can you confirm that the i18ned version does not display the times correctly for dates in December?

[shankari]

@atton16 given that this is a generic issue, I think you should fix it in getLocalTimeString instead of in the current location. You also need to change moment2localdate to increment the month by 1 (e.g. month: currMoment.month() + 1,) to make the draft trips consistent with the retrieved trips.

Please also submit this fix to master. Master can't use the user profile fixes obviously.

[shankari]

At a high level, I am not opposed to making the months zero-indexed in the localdate. I just think that it should be consistent across processed and draft trips. moment uses zero indexed months to be consistent with the Date constructor, but they do say

Note: Because this mirrors the native Date parameters, months, hours, minutes, seconds, and milliseconds are all zero indexed. Years and days of the month are 1 indexed.
This is often the cause of frustration, especially with months, so take note!

Since we also support searches by local date, let's not pass on this frustration to our end users, who don't care about the Date constructor. I vote that we retain 1 indexing for months within e-mission and fix the months while converting to/from moment.

[shankari]

FYI, moment's decision to use zero indexing has caused a lot of comments/requests for change.
https://github.com/moment/moment/issues?q=is%3Aissue+zero+index+is%3Aclosed
which just validates the decision to stay with 1 indexing for the month in e-mission.

[PatGendre]

Hi @shankari @atton16 @stephhuerre and happy new Year!

However, the date should be displayed as "Invalid" for all of December, so I am still not sure why @PatGendre and @stephhuerre, who are using the FabMob fork with the internationalization changes have not yet noticed this. Maybe they haven't been checking a lot in December because of the holidays?
@PatGendre @stephhuerre can you confirm that the i18ned version does not display the times correctly for dates in December?

I confirm I have the invalid date too in December, sorry for not having noticed it, in part because I tested other versions of the app.
A bit strange though that no other user saw it, even if we have only a few users now.

, in part because I worked on other projects.

[shankari]

Thanks @PatGendre
@atton16 can you make the changes assuming that the e-mission version of local_dt will have 1-indexed months?

Concretely, change both moment2localdate and getLocalTimeString and submit the fix to master as well. LMK if you would like me to generate a draft PR that you can test.

[stephhuerre]

Hi @shankari,

Sorry, I totally missed the notifications on the #615, otherwise, I would have answered...
We did not notice the issue in December, as we only start the trip analysis pipeline in January...

We would be happy to submit an PR, however:

1. I don't understand how to "cleanly" implement your directions to fix the issue:

given that this is a generic issue, I think you should fix it in getLocalTimeString instead of in the current location. You also need to change moment2localdate to increment the month by 1 (e.g. month: currMoment.month() + 1,) to make the draft trips consistent with the retrieved trips.

If we modify moment2localdate, will it not affect both draft and processed trips?

2. We don't have a proper way to test until March 31st as we don't have any draft trips on days that may cause an issue. Can we easily set one of our January 31st trips to draft? Or is there a clever mechanism that will allow us the test :-)

[shankari]

@stephhuerre there is a mismatch between the python and moment implementations of date expansions. We have to pick one, and I pick the python implementation, because the zero indexing in moment seems to generate a lot of confusion.
https://github.com/moment/moment/issues?q=is%3Aissue+zero+index+is%3Aclosed

So the standard for e-mission is that the month will be 1-indexed (so January == 1 ... December == 12).

Given this decision, we need to make two changes.

• We use getLocalTimeString to convert from the e-mission localdate -> moment for display. Since e-mission will have 1 indexed months, and moment expects 0 indexed months, we will need to subtract 1 from the month before calling moment(dt)
• This will work for all processed trips. For draft trips, since the localdate is not created on the server, we create it from the timestamp using ... moment, using moment2localdate. So, without any additional changes, the local date for draft trips alone will be zero indexed, and will not meet the standard. In order to be compatible with the standard that we decided on, you will have to change moment2localdate also to add 1 to the month and make it be 1-indexed.

Please let me know if this is still not clear and I will attach a patch for you to test since that is easier than a more detailed description.

For testing, I would suggest:

• setting up a staging server (maybe on the developers' laptop)
• downloading your January data, either from the server or from your phone
• loading the data onto the staging server
• connecting the UX with the changes (using the devapp) to the staging server

https://github.com/e-mission/e-mission-server#loading-test-data

[shankari]

I have a pending PR to load data to a remote staging server (e-mission/e-mission-server#726) that has been languishing because I am busy. If that will help you, please vote for it! If it gets sufficient votes, I will move it up my dev queue.