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

Implement award points page #122

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Implement award points page #122

wants to merge 3 commits into from

Conversation

alexzhang1618
Copy link
Contributor

Info

Closes [ISSUE NUMBER].

[description]

Changes

  • Implement award points functionality

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)
  • Logistics Change (A change to a README, description, or dev workflow setup like
    linting/formatting)
  • Continuous Integration Change (Related to deployment steps or continuous integration
    workflows)
  • Other: (Fill In)

Testing

I have tested that my changes fully resolve the linked issue ...

  • locally on Desktop.
  • on the live deployment preview on Desktop.
  • on the live deployment preview on Mobile.
  • I have added new Cypress tests that are passing.

Checklist

  • I have performed a self-review of my own code.
  • I have followed the style guidelines of this project.
  • I have documented any new functions in /src/lib/* and commented hard to understand areas
    anywhere else.
  • My changes produce no new warnings.

Screenshots

Copy link

vercel bot commented Jan 16, 2024

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

Name Status Preview Comments Updated (UTC)
membership-portal-ui-v2 ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 30, 2024 8:20am

Copy link
Member

@farisashai farisashai left a comment

Choose a reason for hiding this comment

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

I had a better idea after the last PR I did for the admin page - check box that says "Award to all Users" and if it's true it disables the user field and creates a milestone instead since it's technically the same award points functionality under a different name. That way we condense 2 pages in one

@@ -20,8 +22,10 @@ const AwardPointsPage: NextPage = () => {
formState: { errors },
} = useForm<FormValues>();

const onSubmit: SubmitHandler<FormValues> = () => {
// TODO
const onSubmit: SubmitHandler<FormValues> = async ({ email, description, points }) => {
Copy link
Member

Choose a reason for hiding this comment

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

wrap this in an admin event manager function for repo conventions and also error handling

Copy link
Member

@SheepTester SheepTester left a comment

Choose a reason for hiding this comment

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

looks fine to me. I think it's a bit weird that EventAPI.awardBonusPoints directly returns the response object rather than the emails Promise<string[]>, but at the same time I think it'd be confusing if a function named awardBonusPoints returned a list of emails


const requestBody: CreateBonusRequest = {
bonus: {
users: [user],
Copy link
Member

Choose a reason for hiding this comment

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

Maybe you should have awardBonusPoints accept a list of users since the backend supports it, and the function returns a list of emails rather than just one


export const awardBonusPoints = async (
token: string,
user: string,
Copy link
Member

Choose a reason for hiding this comment

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

Is this a username or user UUID?

Suggested change
user: string,
username: string,
Suggested change
user: string,
user: UUID,

(or userId: UUID)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants