-
Notifications
You must be signed in to change notification settings - Fork 1
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
feat: only allow applications in application period #53
feat: only allow applications in application period #53
Conversation
c903c96
to
b77b267
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice 📈 Will test it locally tomorrow 🧪
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice changes, good catch on the end date for period status check.
Can't test it locally before the test-user is authorized again in the dev-environment, so can't approve the PR yet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code changes behaves as expected when tested locally. Nice!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! I can see we have some issues with error-handling. E.g. when the application period does not exist. I also noticed if you send a bad request (e.g. no body) to the applications-endpoint it crashes the app 👀
Can be partially fixed by adding a proper catch with some responses after .then in postApplication, but its a tiny bit hacky 😕
E.g.
.catch((err) => { if(err.name === 'ValidationError'){ return res.status(400).json({ message: err.message }) } return res.status(500).json({ message: "Unable to save application" }) })
This reverts commit 6bcb74c.
* added catch-clause in postApplication
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⭐
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🚀 @SanderArntzen, can you test one last time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error-handling for when application period doesn't exist, and sending a uncomplete body to POST application, works now. Good job 👍
Closes #43
Summary of changes
applicationperiod
as collection instead of defaulting toapplicationperiods