BIDMCDigitalPsychiatry / LAMP-platform

The LAMP Platform (issues and documentation).
https://docs.lamp.digital/
Other
13 stars 10 forks source link

Activity Timezones #417

Closed SuruPa00 closed 2 years ago

SuruPa00 commented 2 years ago

When activities are scheduled, they default to EST. currently on the dashboard there is no way to specify timezone, or to see what timezone it is in. We want to select the timezone OR if we do not select it, make it the user’s current timezone.

From a UI perspective, this could be set up so that when there is a schedule for an activity and you see the boxes for a start date, time, interval, and custom times, there can be a fifth box called "Time Zone" that takes the time as a +/- number from GMT. This would be implemented so that only +/- numbers can be inputted. Ex. the box could have -5 GMT to specify the EST time zone. This would also require a resizing of the other boxes. See screenshot for where it could be placed below.

image

avaidyam commented 2 years ago

To simplify the implementation of this feature (technically a bug), let's change the behavior to use the user's current timezone.

  1. When viewing the Feed in the Dashboard/App, the current timezone can be requested from the browser.
  2. For scheduling push notifications, the server should determine the current timezone based on the latest GPS point automatically.
    • This change should be entirely implemented within LAMP-worker, and should require no new user interface changes.

Should we require more advanced scheduling (a deferred but often requested feature), see implementation details in #269.

Linoy339 commented 2 years ago

@avaidyam. Just to clarify:

  1. When viewing the Feed in the Dashboard/App, the current timezone can be requested from the browser.

Is the timezone which we enter here to be saved as new sensor_event like :

{
_parent:U123456789,
sensor:"lamp.timezone"
data:{
timezone:"GMT +5"
}
}

or as tag which would be :

{
_parent:U123456789,
type:"me",
key:"lamp.timezone",
value:"GMT +5"

}
  1. For scheduling push notifications, the server should determine the current timezone based on the latest GPS point automatically.

Suppose, We have gps data for a particular participant, say :

             {
                "latitude": 9.825516752275586,
                "longitude": 76.3456266651752,
                "accuracy": 4.780498660057227,
                "altitude": 7.80199283733964
            }

With the latitude and longitude above, we need to find the timezone. Right? If yes, we might have to use particular api (eg:google map api) or npm package(eg:node-geo-tz) to find out the timezone, which seems to be an overhead.

It seems better to send timezone together with co-ordinates from the app side, so that we can fetch it from sensor_event as lamp.gps. Please confirm.

avaidyam commented 2 years ago

You present a good point with the difficulty of reverse geocoding the GPS data to a time zone. I agree that it makes more sense to cache the time zone as a Tag upon loading the dashboard, and then using that instead. This tag should only be updated if the time zone has changed or if no time zone tag was previously created. If this tag doesn't exist when the LAMP-worker is attempting to send push notifications, it should default to UTC.

Linoy339 commented 2 years ago

Thanks for the clarification @avaidyam . How often the tag should be updated, as there is a chance of change in time zone if the user travels ? Is it to be updated only while login or do a periodic time zone update has to take place ? Can you suggest?

avaidyam commented 2 years ago

For now, only upon login.

Linoy339 commented 2 years ago

Okay. Thanks

Linoy339 commented 2 years ago

@avaidyam : Just to clarify: The requirement is to have notifications for the same time in any time zone. Right? We can explain with a scenario here:

Suppose there are 2 participants for a particular study. One of them is in India and other is in Toronto(Canada). If a researcher from India have set notification at 3.00 PM, one of the participants get it at 3.00PM and other will be getting it at 4.20AM(Toronto time). This is what is happening now.

The change which is to be implemented is that both users should get notification on 3.00pm irrespective of the time zone, which means one user will get it at 3 pm (Indian Time)and other will also get it at 3 pm(Toronto Time).

Can you clarify or confirm it?

avaidyam commented 2 years ago

Yes, that's correct.

Linoy339 commented 2 years ago

Thanks @avaidyam . In this case, we also need to save time zone for the particular activity as :

{
_parent:{activity_id},
type:"me",
key:"lamp.activity.timezone",
value:"GMT +5"
}

This is required to find the local time zone where the activity got saved and from this, we can find the local time of that particular time zone. Thus, we can convert this time to utc time of particular time zone where the user belongs to.

If we take the above scenario( 3.00 PM notification for Indian and Canada users ), activity time zone will be Indian and utc time saved would be 9.30 AM. Also the lamp.activity.timezone would be GMT+5.30. From this we can find the local time as 3.00 PM. The user from canada will be having a time zone offset in the form of tag lamp.participant.timezone as GMT+5.00. Thus, we can find out the utc time for the user as 20.00pm (3.00 PM +5.00= 8.00 PM). Thus both users gets notification at 3.00 pm irrespective of the time zone

Linoy339 commented 2 years ago

@avaidyam . This is clarified. But, we need some more clarity on activity time zone: As the researchers can have varied time zones, it is required to save schedule based time zones. So, can we have time zone to be saved in schedule object in activity model instead of tag lamp.activity.timezone ? This makes easier to find out the local time and schedule based on participant's time zone.

Please let us know on this

avaidyam commented 2 years ago

Timezone should only be saved on the participant side, once. The schedule is timezone-agnostic and will be translated to the patient timezone at runtime by the LAMP-worker before scheduling and sending push notifications.

sarithapillai8 commented 2 years ago

@avaidyam Sorry, This is confusing. When a schedule is added with a time 3pm. The notifications and feed row should show with the time 3pm in every timezone, right? The issue with this is we are saving UTC of this time. Instead if we are just saving the time string we could just use that string in all the timezones without any conversions. As we are saving UTC we need to find which was the scheduling user's(Researcher/admin) timezone while scheduling so that we can find the time he actually used. Otherwise with the UTC, how can we find which time they actually scheduled if the participant is in another timezone? Please let us know your thoughts.

sarithapillai8 commented 2 years ago

@avaidyam Please reply to the above comment so that we can start on this.

avaidyam commented 2 years ago

@sarithapillai8 Yes, it should show 3pm in every timezone. Researchers are not plugging in a time relative to their timezone, they are ALWAYS dealing with the patient's timezone.

sarithapillai8 commented 2 years ago

@avaidyam Can we just save the time string instead of UTC datetime string ? If this is acceptable, let us know how to handle the existing schedules? Currently UTC converted datetime is saved for schedule times (which is calculated by converting the local timezone of the researcher).

avaidyam commented 2 years ago

You cannot change the current format of the Activity Schedule. Please use the existing data structures and reinterpret them in the dashboard code or server code as required.

sarithapillai8 commented 2 years ago

@avaidyam So there should be some way to understand the actual time selected by the researcher. With the current data, the activity schedule time was saving by converting the time selected by the researcher(which is considered as his local time) to UTC time. So we can just use those UTC values without any conversion as we don't have any information of which time the researcher actually selected while creating the schedule. And for future, remove this UTC conversion and just save the time selected. And use this exact time everywhere without any conversions.

avaidyam commented 2 years ago

Right. While the date-time string has a date, a time, and a timezone, currently we ignore the date. We should also ignore the timezone. There is no other change to the data model this way, and if we support custom time zones in the future then it's possible to use that field again.

sarithapillai8 commented 2 years ago

@avaidyam Please correct me if I am wrong : The researcher selected the time - Mon Dec 13 2021 14:00:45 GMT+0530 (India Standard Time), which was saved as 2021-12-13T08:30:45.820Z. Then we need to ignore the timezone and date, and consider the time '08:30' in every time-zone. I mean in feed and notifications. ie, we just use the time 08.30 without any conversion.

In future save the time in local datetime format new Date(data.time).toLocaleString() and use this time value without any conversion for notification and feed.

If this is the plan we don't need to bother about custom time-zone changes for now.

avaidyam commented 2 years ago

Yes that's correct. No changes to how the Researcher sets the schedule. Now the "8:30" they set refers to the patient's local timezone.

Linoy339 commented 2 years ago

@avaidyam . Just to clarify:

1) In this case, there will be mismatch between time for which the notification got set and the same for which notification got triggered. In the above example, from the researcher's perspective, the notifications were supposed to fire at 2 PM and Instead, it gets fired at 08:30 AM.

2) Also the key time in schedule object would be either 2021-12-13T08:30:45.820Z(old schedules) or 13/12/2021, 22:25:16 pm (new schedules). In both cases, we need to fetch the time from the string. In the above case, we need to fetch time as 08:30 Am from 2021-12-13T08:30:45.820Z and 22:25:00 from 13/12/2021, 22:25:16 pm

avaidyam commented 2 years ago

@Linoy339 @sarithapillai8 The UI should show "8 AM" as if it were UTC, regardless of the Researcher's timezone.

I don't think we were made aware that there is a change to the data format for schedules? When did this occur and why? Please revert to the old format ASAP. You cannot break from ISO 8601 date-time formatting and roll a custom format for JSON and have it still be compatible with JSON libraries - both frontend and backend should ALWAYS use Date (built-in) and the proper toString() format - which is ISO 8601. This needs to be rectified quickly - and existing schedules in the database need to be retroactively fixed.

Linoy339 commented 2 years ago

@avaidyam . Present data format to save a schedule is :

{"schedule": [{"start_date":"2021-10-29T11:33:00.000Z", "time":"2021-10-29T11:34:04.124Z", "custom_time":null, "repeat_interval":"biweekly", "notification_ids":[222496] } ]}

Is this format an issue and to be changed ?

avaidyam commented 2 years ago

@Linoy339 The format you have shared in the comment is correct, however, you must remove notification_ids. That cannot be exposed in any API. I was referring to this comment you made:

Also the key time in schedule object would be either 2021-12-13T08:30:45.820Z(old schedules) or 13/12/2021, 22:25:16 pm (new schedules).

This new schedule format 13/12/2021, 22:25:16 pm is illegal for JSON + ISO 8601. Please do not use this kind of format anywhere. This format - 2021-12-13T08:30:45.820Z - is correct and accepted.

sarithapillai8 commented 2 years ago

@avaidyam We didn't change anything. The format is still the same as ISO 8601 date-time formatting.

So our plan will cause some issues with existing data like - With the current schedules the notifications and feed will work with UTC time, not the time set by the researcher. UI will be changed like the time selected by the researcher will be saved without any conversion in ISO 8601 date-time format and it will be used without any conversion for notifications and feed.

sarithapillai8 commented 2 years ago

@avaidyam @SuruPa00 Do you want the date to be keep in the same way as time? I mean as we take the time string from UTC format saved datetime, do we need to extract the date or do we need to convert it in local timezone?

avaidyam commented 2 years ago

Just the time for now.

sarithapillai8 commented 2 years ago

LAMP-dashboard updates are completed and QA confirmed. The changes will be updated in dashboard-staging after server side changes.

ZCOEngineer commented 2 years ago

Done almost 70%, remaining developer testing and QA testing.

michaelmenon commented 2 years ago

it will be pushed to staging by this week. Have changed it to feature as it as a change form the initial given requirement.

Linoy339 commented 2 years ago

@avaidyam . As we discussed, we will push our code by tomorrow. Before, can you add env variable TIMEZONE and keep the value as 'America/New_York' ? The purpose of this variable is to take it as default value, if no participant time zone is found.

avaidyam commented 2 years ago

Sure, we'll make sure this env var is added to both staging and production stacks for server and worker. //cc @lukeoftheshire

@Linoy339 Just to confirm, however, if this variable is not defined, do the server and worker components use UTC time?

lukeoftheshire commented 2 years ago

@avaidyam @Linoy339

I have added the TIMEZONE env variable with the specified value to all 4 services.

Linoy339 commented 2 years ago

Thanks @lukeoftheshire . Also, I need to inform you that worker instances only required these.

Sure, we'll make sure this env var is added to both staging and production stacks for server and worker. //cc @lukeoftheshire

@Linoy339 Just to confirm, however, if this variable is not defined, do the server and worker components use UTC time?

@avaidyam . Yes, It will take UTC. But its recommended to place this variable for avoiding much complications.

avaidyam commented 2 years ago

Great, thanks!

sarithapillai8 commented 2 years ago

@avaidyam We were planning to update this change on dashboard-staging today. Can we go with it? We need to do QA on dashboard-staging. Hopefully QA can be completed by Monday. In between if any release is planned, could you please let us know?

avaidyam commented 2 years ago

Yes, that's fine - thanks for the heads up!

avaidyam commented 2 years ago

Sharing a status update on this issue below:

@michaelmenon: We are hoping to complete QA for activity Timezone by Monday for, but just in case its not complete we will confirm that by Tuesday.

Linoy339 commented 2 years ago

So our plan will cause some issues with existing data like - With the current schedules the notifications and feed will work with UTC time, not the time set by the researcher. UI will be changed like the time selected by the researcher will be saved without any conversion in ISO 8601 date-time format and it will be used without any conversion for notifications and feed.

FYI:

Just Reminding that older schedules can have issues, because the time saved in scheduler was in utc earlier. Now, its with local timezone of the participant. So, do we need to inform all researchers to change the time accordingly ? or else, there should be some script to update activity schedules by applying some calculation .(eg: say substract 5 hours from existing time)

sarithapillai8 commented 2 years ago

@avaidyam This is updated in dashboard-staging. QA completed.

avaidyam commented 2 years ago

So, do we need to inform all researchers to change the time accordingly?

@Linoy339 This is something my team will handle. Thanks for the heads up!

avaidyam commented 2 years ago

@tlakhtak discovered a scheduling error issue with this:

  1. Using dashboard-staging..., scheduled a survey for 10:29am, with the default start date Dec 31, 1969 (which should be the current date instead, I believe). ⚠️
  2. Received a survey at 10:14am. ⚠️
  3. When clicked edit button and re-saved same schedule as before, received another notification at 10:15am. ❌
  4. When went to add a new schedule and deleted the default hours to put in a new time, got an error TypeError: cannot read properties of null (reading ‘getHours’). ❌
  5. Received another survey notification at 10:17am. ❌
  6. And again at 10:19am. ❌

Further notes around importing activities with schedules:

  1. Preset schedules for activity groups did not import into the same time zones (went from 7pm and 8pm ET to 12am and 11pm respectively ET) ❌
  2. Activity now scheduled for 12am was clickable at 10:31am (typically, activities are only available in the feed for a couple hours after they are scheduled. ❌
  3. Activity scheduled for 11pm was not clickable ahead of time (consistent with the past) ✅
    • In another instance, I scheduled a survey for 10:45am, and it shows up in the calendar settings as scheduled but did NOT show up in the Feed and I did not get a notification. ❌

@Linoy339 Since this seems to be more in the LAMP-worker side, can you comment on whether this is expected and why the issue could be happening?

@sarithapillai8 Can you try to reproduce the TypeError on the LAMP-dashboard side?

Linoy339 commented 2 years ago

@avaidyam. We didn't get such behavior of notifying every minute or so. We have tested with different devices with different timezones and it was workig for every repeat type . Some of the time zones includes New York, India, Auckland. It was working fine.

  • Using dashboard-staging..., scheduled a survey for 10:29am, with the default start date Dec 31, 1969 (which should be the current date instead, I believe). ⚠️

  • Received a survey at 10:14am. ⚠️

  • When clicked edit button and re-saved same schedule as before, received another notification at 10:15am. ❌

  • When went to add a new schedule and deleted the default hours to put in a new time, got an error TypeError: cannot read properties of null (reading ‘getHours’). ❌

  • Received another survey notification at 10:17am. ❌

  • And again at 10:19am. ❌

  • In another instance, I scheduled a survey for 10:45am, and it shows up in the calendar settings as scheduled but did NOT show up in the Feed and I did not get a notification. ❌

-- If start date is yet to happen for the individual participant time zone, it wont notify that participant

Can you please provide the activity id for both?

sarithapillai8 commented 2 years ago

@sarithapillai8 Can you try to reproduce the TypeError on the LAMP-dashboard side?

This is fixed in dashboard-staging.

avaidyam commented 2 years ago

Can you please provide the activity id for both?

@Linoy339 They were fresh activities created in staging.

Linoy339 commented 2 years ago

Okay. It seems, the researcher is Tanvi . Right?

avaidyam commented 2 years ago

That should be correct. //cc @tlakhtak

Linoy339 commented 2 years ago

There are no schedules now for any of these activities. I think, all the schedules has been deleted. We believe, no notifications are firing now for all these. @tlakhtak . Can you please add schedules as you did last week?

tlakhtak commented 2 years ago

I received notifications last night at 11pm for Evening activity and 12am for Cognitive Games activity. Both activities still have scheduled activities in the Feed, for 7pm and 8pm today respectively. This was a test user using dashboard-staging.lamp.digital, server: api-staging.lamp.digital, and user U8950615963

Linoy339 commented 2 years ago

@tlakhtak . Yes. The notifications are supposed to get fired daily 11pm (Evening activity) and 12 AM (Cognitive Games ). So, its working as expected.

Both activities still have scheduled activities in the Feed, for 7pm and 8pm today respectively

It seems to be a cache issue, as I am getting displayed it as 11 PM and 12 AM respectively. Please find the screenshot : feed