Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/calendar-part4 #149

Merged
merged 4 commits into from
Jun 20, 2024
Merged

Feature/calendar-part4 #149

merged 4 commits into from
Jun 20, 2024

Conversation

timothyrusso
Copy link
Collaborator

Description

This task will handle disabled states for weekly checkin in the event panel. Past checkins will be disabled.

Issue link

Fixes # (issue)

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

Copy link

vercel bot commented Jun 14, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
chingu-dashboard ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 20, 2024 7:06pm

Copy link
Collaborator

@Dan-Y-Ko Dan-Y-Ko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be working fine but I have one concern. @JaneMoroz I think we should consolidate the use of current date to one place. Right now we're just creating new Date all over the place (for current date). There is a current date in the user store currently. Would it be ok to just make use of that for all cases of current date that is needed?

@JaneMoroz
Copy link
Collaborator

JaneMoroz commented Jun 19, 2024

@Dan-Y-Ko @timothyrusso If I understand correctly, we've decided to convert everything to the user's timezone (the one we store in the database), regardless of the user's current location. In the future, we plan to implement a function that compares the user's timezone in the database with the timezone on the client. If the timezones differ, a modal will appear, offering the user the option to update their timezone settings. Dan, is that correct?

Right now we use new Date() frequently in the calendar and almost always on the client side. We get all dates in the client's timezone, not in the user's timezone stored on the server. And this can actualy cause different bugs, if the client timezone is not the same as the one the user has on the server. So one bug I can think of straight away:

  • A voyage's start/end date is shown according to the client's timezone, but a meeting is shown according to the timezone stored in the database. As a result, a user might see that the sprint meeting is outside of the sprint boundaries or even outside of voyage boundaries.

So to better understand what I'm talking about you need to use chrome browser and change your client timezone to some extreme case, I chose GMT -11 timezone. So here's an instruction:

  • open dev tools
  • click three vertical dots ("Customize and control DevTools")
  • click "More Tools" from the dropdown
  • click "Sensors"
  • then in Location choose "others" and click "manage" buttons => then "Add location" => set location name to "Alofi" and "Timezone Id" to "Pacific/Niue"=> click save, close it
  • then in sensors, you can set location to alofi
  • you can also use console.log(new Date()) somewhere in Calendar.logic and just check the timezone in the console

Now you can see everything is weird in the calendar, basically because our calendar uses both timezones right now. It uses the client timezone and also the user's timezone from the database through convertStringToDate helper function. But if I understand correctly it should use the user's timezone from the database only. So we need one more helper function to do all this conversion and probably use it everywhere we use new Date.

Btw in case of Jessica, if you want to see how calendar looks like in her timezone, you can add one more location (set location name to "Melbourne" and "Timezone Id" to "Australia/Melbourne") and then just switch between Melbourne and Alofi.

@JaneMoroz
Copy link
Collaborator

JaneMoroz commented Jun 19, 2024

This seems to be working fine but I have one concern. @JaneMoroz I think we should consolidate the use of current date to one place. Right now we're just creating new Date all over the place (for current date). There is a current date in the user store currently. Would it be ok to just make use of that for all cases of current date that is needed?

If you mean getCurrentSprint function, the current date we generate there is in UTC, because we call it on the server. However, if we call getCurrentSprint on the client (which we currently don't), we will still get the current sprint right, because currentDate will use the client's timezone but the sprint's start and end dates will also use the client's timezone (date-fns library will automatically convert ISOstring to Date when isWithinInterval is called and ISOstrings are passed as props). btw because getCurrentSprint is called on the server most of the time, we can't use the store in this case.

But I agree if we have a client component which needs the current date, it should get it from the store. But right now we simply get the current date in the client timezone, probably we need to convert it and only then store it idk

@Dan-Y-Ko
Copy link
Collaborator

@Dan-Y-Ko @timothyrusso If I understand correctly, we've decided to convert everything to the user's timezone (the one we store in the database), regardless of the user's current location. In the future, we plan to implement a function that compares the user's timezone in the database with the timezone on the client. If the timezones differ, a modal will appear, offering the user the option to update their timezone settings. Dan, is that correct?

Yes

@Dan-Y-Ko Dan-Y-Ko merged commit 10bd212 into dev Jun 20, 2024
3 checks passed
@Dan-Y-Ko Dan-Y-Ko deleted the feature/calendar-part4 branch June 20, 2024 19:13
Dan-Y-Ko added a commit that referenced this pull request Jul 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants