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

Restart Challenge #37

Open
wants to merge 1 commit into
base: development
Choose a base branch
from

Conversation

xiaoshaotongzhi
Copy link
Contributor

Checklist

Please ensure you have followed all of the steps below before submitting a pull request:

  • Include the corresponding Jira issue key and #done in the PR title, like so: "JRA-123 #done Migrate Election Reminders"
  • Verify that the code compiles (npm run dev)
  • Verify that the project builds (npm run build)
  • Verify that all tests pass
  • Verify that unit tests cover 100% of the code
  • Create Storybook stories for visual components
  • Verify that any visual components match the Figma
  • Test with a screen reader (if applicable)
  • Document your code with TSDoc comments
  • Format your code with Prettier

Overview

REQUIRED
Fill in the overview of this PR, what this PR is trying to achieve.

Test Plan

REQUIRED
What you did to verify your PR works as claimed? Make sure to list things/steps/screenshots/screentcasts so that others can reproduce your test easily. Share styling changes and component updates through screenshots.

Follow ups

It is okay for the PR to be not perfect, list what you/other should work on after this PR is merged.

Copy link
Contributor

@dvorakjt dvorakjt left a comment

Choose a reason for hiding this comment

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

Looks very good. There are some minor nits to take care of, and you will need to write tests for the code you've added. See the comments that I added to each file for more details. That's about all I see, so once you take care of those things, we'll be able to merge this :)


try {
const data = await request.json();
const { userId } = restartChallengeSchema.parse(data);
Copy link
Contributor

Choose a reason for hiding this comment

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

^ I don't think this route actualy needs a schema, or to read any data from the request body, for that matter. I think you can just use the Auth class to get the user from cookies.


const user = await auth.loadSessionUser();
// impossible that frontend and cookie issue.
if (!user || user.uid !== userId) {
Copy link
Contributor

Choose a reason for hiding this comment

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

^ Please remove vestigial comments such as this one and the one on line 24

return NextResponse.json({ error: e.message }, { status: e.statusCode });
}

return NextResponse.json({ error: 'Bad data.' }, { status: 400 });
Copy link
Contributor

Choose a reason for hiding this comment

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

Because this route will no longer be parsing a request body, this catch all statement should probably be a 500 error code with a more general error message.

try{
setLoading(true);
await restartChallenge();
} catch (error) {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe use the AlertsContext to show the user an error message if this fails?

}

return updatedChallengeEndTimestamp;
//follow the style of getUserById AND supabase->throw an error
Copy link
Contributor

Choose a reason for hiding this comment

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

^ Please remove this comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you tidy up the vertical whitespace in this file, please?

Copy link
Contributor

Choose a reason for hiding this comment

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

The button component has been moved to src/components/utils/button, so all the folder src/components/button and all its contents can safely be deleted.

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.

2 participants