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

fix: add missing validation to admission period backend #66

Merged
merged 5 commits into from
Apr 6, 2022

Conversation

SanderArntzen
Copy link
Contributor

@SanderArntzen SanderArntzen commented Mar 31, 2022

Closes #52

Summary of changes

  • add validation to admission period backend (formatting the dates received by the backend to "YYYY-MM-DD" and checking that the date is valid, and check that the start date is before the end date
  • add format to the dates sent from frontend, so they just contain the "date-part", not the "time-part"

Testing

The code can be tested both through frontend and Postman.

@SanderArntzen SanderArntzen added bug 🐞 Something isn't working backend 🍑 Backend tasks labels Mar 31, 2022
@SanderArntzen SanderArntzen linked an issue Mar 31, 2022 that may be closed by this pull request
@SanderArntzen SanderArntzen self-assigned this Mar 31, 2022
Copy link
Collaborator

@LiviaValenti LiviaValenti left a comment

Choose a reason for hiding this comment

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

I am not able to use the frontend with these changes in backend. 😮 If the backend is to expect the format YYYY-MM-DD validating it through regex, you have to update the frontend to send it in that format. ✂️

Another alternative is to find another way to validate the date completely. I don't have any tips at the moment for that, but I know handling dates are tricky 😅

backend/controllers/admissionPeriodController.ts Outdated Show resolved Hide resolved
backend/controllers/admissionPeriodController.ts Outdated Show resolved Hide resolved
@SanderArntzen
Copy link
Contributor Author

I am not able to use the frontend with these changes in backend. 😮 If the backend is to expect the format YYYY-MM-DD validating it through regex, you have to update the frontend to send it in that format. ✂️

Another alternative is to find another way to validate the date completely. I don't have any tips at the moment for that, but I know handling dates are tricky 😅

I will take a closer look on the format for how the frontend sends tha dates. The reason for using regex was that I haven't found a good way to validate date yet. Validating dates are some tricky stuff

@eivindkopperud
Copy link
Member

eivindkopperud commented Apr 1, 2022

I will take a closer look on the format for how the frontend sends tha dates. The reason for using regex was that I haven't found a good way to validate date yet. Validating dates are some tricky stuff

Since you have the ISO 8601 format on the datestrings, you can parse it with const date = new Date(theString). If the string is valid, it will create a date object. If not, it will cause some sort of exception. Then you can call date.toISOString() to get it in the format YYYY-MM-DDTHH:MM:SS. If you only want the first part, you can do .split("T") or do a .substring.

The advantage I see for this approach is that the frontend doesn't need to care about what format it sends, as long as it at least has YYYY-MM-DD. The con is that strings such as 200-01-01 is a valid datetime (January 1st, 200), which might cause weird behavior

I won't necessarily say its more clean, but it is a solution!

@SanderArntzen
Copy link
Contributor Author

I will take a closer look on the format for how the frontend sends tha dates. The reason for using regex was that I haven't found a good way to validate date yet. Validating dates are some tricky stuff

Since you have the ISO 8601 format on the datestrings, you can parse it with const date = new Date(theString). If the string is valid, it will create a date object. If not, it will cause some sort of exception. Then you can call date.toISOString() to get it in the format YYYY-MM-DDTHH:MM:SS. If you only want the first part, you can do .split("T") or do a .substring.

The advantage I see for this approach is that the frontend doesn't need to care about what format it sends, as long as it at least has YYYY-MM-DD. The con is that strings such as 200-01-01 is a valid datetime (January 1st, 200), which might cause weird behavior

I won't necessarily say its more clean, but it is a solution!

Thanks for the suggestions, I have used some of them in the new version of the code

Copy link
Member

@Xtrah Xtrah left a comment

Choose a reason for hiding this comment

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

Can't test this without updated fixtures 😁

When I load now admissionPeriodResponse is { start_date: undefined, end_date: undefined }. Can you check that frontend loads and works for you after you update and load fixtures?

@SanderArntzen SanderArntzen force-pushed the fix/validate-admission-period-backend branch from b49655e to 3661bfa Compare April 5, 2022 15:07
@SanderArntzen
Copy link
Contributor Author

Can't test this without updated fixtures 😁

When I load now admissionPeriodResponse is { start_date: undefined, end_date: undefined }. Can you check that frontend loads and works for you after you update and load fixtures?

I have now changed the code so it works with the fixture. It also works through frontend

@SanderArntzen SanderArntzen requested a review from Xtrah April 5, 2022 15:14
Copy link
Collaborator

@LiviaValenti LiviaValenti left a comment

Choose a reason for hiding this comment

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

Sure, one can send in silly values like 2420-69-69, but at least it does not crash anything. 🥂

It also sends nice error-messages back and works as expected. I suggested a small improvement to the commenting, but either way you have done a great job 🎇

LGTM 🚀

backend/controllers/admissionPeriodController.ts Outdated Show resolved Hide resolved
@SanderArntzen
Copy link
Contributor Author

Sure, one can send in silly values like 2420-69-69, but at least it does not crash anything. 🥂

It also sends nice error-messages back and works as expected. I suggested a small improvement to the commenting, but either way you have done a great job 🎇

LGTM 🚀

Oh, nice you found that. Originally there was a check for non-existing dates, but it must have gone missing somewhere. I have implemented it again in the latest fix-commit

@SanderArntzen SanderArntzen merged commit c2c24a0 into dev Apr 6, 2022
@SanderArntzen SanderArntzen deleted the fix/validate-admission-period-backend branch April 6, 2022 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend 🍑 Backend tasks bug 🐞 Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing validation in application period backend
4 participants