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/sprints functionality pt4 #136

Merged
merged 68 commits into from
Jun 7, 2024
Merged

Conversation

JaneMoroz
Copy link
Collaborator

@JaneMoroz JaneMoroz commented May 14, 2024

Description

  • The weekly check-in form and submit voyage form are fetched from the server now, and then a corresponding validation schema is created.
  • Some components related to the form have been moved to the global components folder.
  • Certain helper functions have been moved to /utils/form, along with validation rules.
  • Added submit functionality to weekly check-in form (can only be submitted when Did you deploy to Production at the end of this Sprint is yes, but the backend team is fixing it => fixed!)
  • Added submit functionality to submit voyage form (not working currently, need to re-seed the dev database first).
  • The status of checking form submission is used to properly render sprint action buttons. Additionally, if the weekly check-in form has already been submitted, the user gets redirected to the corresponding sprint page.
  • If the weekly check-in form is submitted successfully, the user is redirected to the corresponding sprint page, and a success modal is displayed.

In the next PR:

  • fix submit voyage form functionality (if anything is not working)
  • use the status of voyage form submission to render sprint action buttons etc
  • update storybook

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

@Dan-Y-Ko
Copy link
Collaborator

Dan-Y-Ko commented Jun 4, 2024

Users should not be able to submit a checkin for past sprints (past the deadline)

Is it better to redirect from the form straight away or let a user fill the form first but show the error if the user tries to submit (+ maybe show some alert message in the form banner)?

And what about the Submit Check In button, should I dissable it?

I think redirect would be better. Also, yes, disable the button.

@Dan-Y-Ko
Copy link
Collaborator

Dan-Y-Ko commented Jun 4, 2024

I think the short answer questions in the weekly checkin shouldn't be required (the one with the text area inputs).

You want last 4 questions to be optional?

Update: They're required (last 5 not 4), It's what Cheryl said.

in voyage submission form, the voyage showcase video input should be optional. The text in the button in the voyage submission form is incorrect.

Fixed it, we had some bugs in validateInput

I do want it to be optional. They are all phrased in a way that's optional. It's silly to force user to write "n/a" or something like that.

@cherylli
Copy link
Collaborator

cherylli commented Jun 4, 2024

I think the short answer questions in the weekly checkin shouldn't be required (the one with the text area inputs).

You want last 4 questions to be optional?
Update: They're required (last 5 not 4), It's what Cheryl said.

in voyage submission form, the voyage showcase video input should be optional. The text in the button in the voyage submission form is incorrect.

Fixed it, we had some bugs in validateInput

I do want it to be optional. They are all phrased in a way that's optional. It's silly to force user to write "n/a" or something like that.

we can change to optional but it shouldn't really affect the implementation, you should be able to handle both required and non require questions based on the question data. I don't understand what the fuss is, all we have to do is the change the isRequired in the database.

I think the bigger issue is the check in form is very different now, especially for product owners and scrum masters, they have a completely different form it seems (This is a recent change)

@JaneMoroz
Copy link
Collaborator Author

@Dan-Y-Ko

  • added redirect when a user tries to access a weekly checkin form not for the current sprint. So have to make additional fetchSprints server call to get the currentSprintNumber.
  • had to add default values in createValidationSchema. Possibly it will be refactored in future, if we have some editing functionality for this kind of forms. Before defaultValues, zod throwed an error like "expected a string but got null", I tried to use zod's transform and preprocess but then refine didn't want to do things right (This field is required logic)... Adding defaultValue prop to inputs didn't do much either. So this is it for now.

@Dan-Y-Ko
Copy link
Collaborator

Dan-Y-Ko commented Jun 5, 2024

ok, so I'm not sure if this is a bug or it's working as intended. If I change the date manually in the code (changing to a past date seems to work fine but changing to future date doesn't. I changed to june 12 and I get that behavior), the checkin and voyage submission doesn't work. It just redirects to the dashboard page.

Also, I see a lot of code like this written out manually

const voyageTeamMemberId = user?.voyageTeamMembers.find(
          (voyage) => voyage.voyageTeam.voyage.status.name == "Active",
        )?.id;

But we have this already in the getCurrentVoyageTeam util file. You should be able to re-use that.

Also, if I change the date manually to something like new Date(2024, 7, 12), the dashboard just completely breaks. Is that a date-fns thing or some kind of issue somewhere in the code? This is the error I get https://i.imgur.com/keBqAuq.png

@JaneMoroz
Copy link
Collaborator Author

JaneMoroz commented Jun 6, 2024

ok, so I'm not sure if this is a bug or it's working as intended. If I change the date manually in the code (changing to a past date seems to work fine but changing to future date doesn't. I changed to june 12 and I get that behavior), the checkin and voyage submission doesn't work. It just redirects to the dashboard page.

I forgot to remove an additional check related to meetingId in SprintActions. Fixed it!

Also, I see a lot of code like this written out manually

const voyageTeamMemberId = user?.voyageTeamMembers.find(
          (voyage) => voyage.voyageTeam.voyage.status.name == "Active",
        )?.id;

But we have this already in the getCurrentVoyageTeam util file. You should be able to re-use that.

Okay, just updated it a little bit, added voyageTeamMemberId, because currentTeam is a boolean right now.

Also, if I change the date manually to something like new Date(2024, 7, 12), the dashboard just completely breaks. Is that a date-fns thing or some kind of issue somewhere in the code? This is the error I get https://i.imgur.com/keBqAuq.png

new Date(2024, 7, 12) is a bad choice, because the current voyage is till 2024-06-17. And new Date(2024, 7, 12) is not July 12, it's actually August 12, because new Date() takes The month as a number between 0 and 11 (January to December). So just try new Date(2024, 5, 12) - June, 12 and everything should work.

@Dan-Y-Ko
Copy link
Collaborator

Dan-Y-Ko commented Jun 6, 2024

new Date(2024, 7, 12) is a bad choice, because the current voyage is till 2024-06-17. And new Date(2024, 7, 12) is not July 12, it's actually August 12, because new Date() takes The month as a number between 0 and 11 (January to December). So just try new Date(2024, 5, 12) - June, 12 and everything should work.

I was testing to see what would happen if I change the date to something that's outside the voyage dates so I am aware of that part. I was thinking if the date is outside the voyage dates, it should behave the same way as if a user tries to access something they don't have access to, and redirect to the dashboard page or something like that, but instead the app just completely breaks.

@JaneMoroz
Copy link
Collaborator Author

JaneMoroz commented Jun 7, 2024

new Date(2024, 7, 12) is a bad choice, because the current voyage is till 2024-06-17. And new Date(2024, 7, 12) is not July 12, it's actually August 12, because new Date() takes The month as a number between 0 and 11 (January to December). So just try new Date(2024, 5, 12) - June, 12 and everything should work.

I was testing to see what would happen if I change the date to something that's outside the voyage dates so I am aware of that part. I was thinking if the date is outside the voyage dates, it should behave the same way as if a user tries to access something they don't have access to, and redirect to the dashboard page or something like that, but instead the app just completely breaks.

Well, I think the primary reason the app breaks if you try to change date like this in getSprintsSection is because if the current date is outside of the voyage dates, we shouldn't get the voyage data at all (and getSprintsSection isn't triggered at all if no voyage data gets returned from the server), because the voyage's status should be 'Inactive' ... but we still get this data because right now we have at least one Active voyage, no matter how we change the current date. btw this currentDate we get on the server not client, so a user can't manipulate it 😅

So... hmmm... idk 🤔 If you want I can add some redirects but I guess I'll have to add them to the user dashboard too... but again I think this scenario seems unlikely (that a user somehow can be outside the voyage date while the voyage is still 'active.. okay it's possible if we have some bug in our code related to date-time conversion, all this timezone thing, but again the bug itself should better be fixed instead)

@JaneMoroz
Copy link
Collaborator Author

@Dan-Y-Ko btw Jessica has two Active voyages right now, but she can access only one. I think we might want to update getCurrentVoyageTeam to account for this case as well, but maybe in a separate PR?

@Dan-Y-Ko
Copy link
Collaborator

Dan-Y-Ko commented Jun 7, 2024

@Dan-Y-Ko btw Jessica has two Active voyages right now, but she can access only one. I think we might want to update getCurrentVoyageTeam to account for this case as well, but maybe in a separate PR?

That is something that is being discussed for p2. There are cases where a user can be part of multiple teams in a voyage, so we have to account for that. For this initial beta, that won't be the case, so it shouldn't be a problem for now

@Dan-Y-Ko
Copy link
Collaborator

Dan-Y-Ko commented Jun 7, 2024

new Date(2024, 7, 12) is a bad choice, because the current voyage is till 2024-06-17. And new Date(2024, 7, 12) is not July 12, it's actually August 12, because new Date() takes The month as a number between 0 and 11 (January to December). So just try new Date(2024, 5, 12) - June, 12 and everything should work.

I was testing to see what would happen if I change the date to something that's outside the voyage dates so I am aware of that part. I was thinking if the date is outside the voyage dates, it should behave the same way as if a user tries to access something they don't have access to, and redirect to the dashboard page or something like that, but instead the app just completely breaks.

Well, I think the primary reason the app breaks if you try to change date like this in getSprintsSection is because if the current date is outside of the voyage dates, we shouldn't get the voyage data at all (and getSprintsSection isn't triggered at all if no voyage data gets returned from the server), because the voyage's status should be 'Inactive' ... but we still get this data because right now we have at least one Active voyage, no matter how we change the current date. btw this currentDate we get on the server not client, so a user can't manipulate it 😅

So... hmmm... idk 🤔 If you want I can add some redirects but I guess I'll have to add them to the user dashboard too... but again I think this scenario seems unlikely (that a user somehow can be outside the voyage date while the voyage is still 'active.. okay it's possible if we have some bug in our code related to date-time conversion, all this timezone thing, but again the bug itself should better be fixed instead)

Ok sounds good, maybe we can just put this on the side for now

@Dan-Y-Ko Dan-Y-Ko merged commit 2207a4c into dev Jun 7, 2024
3 checks passed
@Dan-Y-Ko Dan-Y-Ko deleted the feature/sprints-functionality-pt4 branch June 7, 2024 22:21
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