-
Notifications
You must be signed in to change notification settings - Fork 8
Feat/create application review #944
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
base: dev
Are you sure you want to change the base?
Conversation
…ackmcgill/hackerAPI into feat/create_application_review
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.
Looks good!! This is super detailed, thorough, and works perfectly. Thank you so much. See my comment on one of the files about the modularity and duplicate logic, I think that is true throughout many of these files here. If we can reduce duplicate functions for each individual field and make the logic for app review more modular overall that would be great.
| const HACKER_REVIEWER_STATUS_STRONG = 'Strong'; | ||
| const HACKER_REVIEWER_STATUS_OUTSTANDING = 'Outstanding'; | ||
| const HACKER_REVIEWER_STATUS_WHITELIST = 'Whitelist'; | ||
| const HACKER_REVIEWER_STATUSES = [ |
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.
non-blocking:
instead of doing this as a list of variables, couldn't lines 28-44 be wrapped into one {} object?
ie. HACKER_REVIEW_STATUSES = {NONE : 'None', POOR : 'Poor', etc.}
| getHackers() | ||
| elif BATCH_ACTION == 'acceptHackers': | ||
| acceptFromEmails() | ||
| elif BATCH_ACTION == 'updateReviewerStatus': |
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.
We have actions and functions for each individual field that we've added to the hacker's profile in the db. Because the fields for reviewers 1 and 2 are exactly the same, why are we defining all these variables and functions separately for reviewername and reviewername2, reviewercomments and reviewercomments2, etc... ? It seems like the logic for both reviewers could be made more modular, and that would also make it much easier to extend to more reviewers in the future if needed. We have a lot of duplicate code/logic now, which will be hard to maintain. Is this assessment correct? lmk if I'm missing anything there
Tickets:
List of changes:
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
Type of change
Please delete options that are not relevant.
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
Test Configuration:
Firmware version:
Hardware:
Toolchain:
SDK:
Questions for code reviewers?
Checklist: