feat: add batchAccept route to accept multiple hackers at once #629

ManethKulatunge · 2019-12-30T09:16:04Z

Tickets:

HCK-153

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.

New feature (non-breaking change which adds functionality)
This change requires a documentation update

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

Added many integration tests, and also did some manual postman tests.

Questions for code reviewers?

Checklist:

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
My changes generate no new warnings
Listed change(s) in the Changelog
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
I have made corresponding changes to the documentation
Any dependent changes have been merged and published in downstream modules

Ty-Won

Can we write some tests for this new route? The code seems okay at a glance but I would like to have something that checks the functionality

…unction, fix router

pierreTklein · 2020-07-26T05:29:59Z

@loreina @logan-r please have a look now at the code. I finished this code, added tests, and also manually tested.

After this goes in, we will need to do the following:

npm run seed to add the new route
npm run docs to generate the docs

Ty-won is no longer part of HackMcGill

krubenok · 2020-07-26T05:51:12Z

🚨DANGER🚨

We still don't know how to repeatedly fix DB incompatibilities that we suspect are caused by npm run seed. See the great chaos of production roll out circa December 2019.

pierreTklein · 2020-07-26T16:30:00Z

Lemme see if I can rewrite npm run seed to be less dangerous, lol

loreina

lgtm, just had a few comments

also I don't think we should run npm run seed now with its issues. if we keep it as is, we should only run it once a year right before apps launch. it's pending a refactor sometime this year before apps do drop tho @pierreTklein if you can look into it that would be epic ❤️

controllers/hacker.controller.js

routes/api/hacker.js

package.json

services/account.service.js

loreina · 2020-07-26T17:15:17Z

for npm run docs is there a way to trigger between v1, v2, v3? we'd probably have to do some back tracing to config that tho

krubenok · 2020-07-26T17:29:15Z

we should only run it once a year right before apps launch.

I disagree. If we can gain confidence in npm run seed it should be part of the CI/CD for deploying to production.

loreina · 2020-07-26T17:30:00Z

accident 🙃

loreina

sorry didn't see hacker.middleware.js before, looks great !!

middlewares/hacker.middleware.js

pierreTklein · 2020-07-26T21:34:29Z

#652 fixes all of our worries about npm run seed. We should be able to integrate it with CI/CD in the future.

That being said, there is a brief moment when running the script where NOBODY has permissions (we basically do a DROP TABLE; CREATE TABLE), so it's probably worth disabling npm run seed when apps are open / near deadline.

* feat: Add HACKER_UPDATE_BATCH success message * feat: Add promise.allsettled so that we can wait for many promises to complete * feat: Add findByHackerId function to Account * feat: Add updatedHackerBatch controller function, generalize validation function, fix router * fix: resolve bug where requests hang when response is null * feat: set route apiVersion number to be 3.0.0 * feat: create parseAcceptBatch Co-authored-by: Pierre Theo Klein <pierre.klein@mail.mcgill.ca>

ManethKulatunge requested review from Ty-Won and loreina December 30, 2019 09:16

ManethKulatunge changed the title ~~Feature/153 accept multiple hackers~~ Feature/153 accept multiple hackers (batchAccept) Dec 30, 2019

Ty-Won previously requested changes Jan 1, 2020

View reviewed changes

ManethKulatunge added 5 commits January 10, 2020 12:06

Batch Accept done

d9e792a

Route created

76c0b7c

Routes constant edited

e5d09fa

Middleware added

1494814

Sending emails for multiple ids enabled

2722d76

ManethKulatunge force-pushed the feature/153-accept-multiple-hackers branch from 63d8cc8 to 2722d76 Compare January 11, 2020 07:33

ManethKulatunge and others added 11 commits January 11, 2020 15:02

routes.constant.js edited

1236ad2

ensure authorised added

46d0037

Middleware made to handle errors

c6c1355

Req.body.ids check

282a300

Tests written

d70c5c6

'develop' -> feature/153-accept-multiple-hackers

b86bd10

Add HACKER_UPDATE_BATCH success message

0ffda41

Add promise.allsettled so that we can wait for many promises to complete

97a1f19

Add findByHackerId function to Account

5273a55

Add completedBatchUpdate controller function, generalize validation f…

978076d

…unction, fix router

Fix middleware. Add many tests.

4c83f60

pierreTklein requested review from Ty-Won and logan-r and removed request for Ty-Won July 26, 2020 04:53

Fix bug where requests hang when response is null

103d435

loreina approved these changes Jul 26, 2020

View reviewed changes

controllers/hacker.controller.js Outdated Show resolved Hide resolved

routes/api/hacker.js Outdated Show resolved Hide resolved

package.json Show resolved Hide resolved

services/account.service.js Show resolved Hide resolved

loreina changed the title ~~Feature/153 accept multiple hackers (batchAccept)~~ feat: add batchAccept route to accept multiple hackers at once Jul 26, 2020

loreina closed this Jul 26, 2020

loreina reopened this Jul 26, 2020

pierreTklein added 2 commits July 26, 2020 16:04

rename completedBatchUpdate to updatedHackerBatch

691e4c8

Set apiVersion number to be 3.0.0

cd558b5

loreina approved these changes Jul 26, 2020

View reviewed changes

middlewares/hacker.middleware.js Show resolved Hide resolved

Create and then use parseAcceptBatch instead of parseAccept

e14f9b0

Merge branch 'develop' into feature/153-accept-multiple-hackers

23d8a8b

pierreTklein merged commit 28e074f into develop Jul 27, 2020

pierreTklein deleted the feature/153-accept-multiple-hackers branch July 27, 2020 01:58

loreina mentioned this pull request Jul 30, 2020

release: v3.0.0 #655

Merged

16 tasks

feat: add batchAccept route to accept multiple hackers at once #629

feat: add batchAccept route to accept multiple hackers at once #629

Uh oh!

Conversation

ManethKulatunge commented Dec 30, 2019 • edited by pierreTklein Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Tickets:

List of changes:

Type of change

How has this been tested?

Questions for code reviewers?

Checklist:

Uh oh!

Ty-Won left a comment

Choose a reason for hiding this comment

Uh oh!

pierreTklein commented Jul 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

krubenok commented Jul 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pierreTklein commented Jul 26, 2020

Uh oh!

loreina left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

loreina commented Jul 26, 2020

Uh oh!

krubenok commented Jul 26, 2020

Uh oh!

loreina commented Jul 26, 2020

Uh oh!

loreina left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pierreTklein commented Jul 26, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ManethKulatunge commented Dec 30, 2019 •

edited by pierreTklein

Loading

pierreTklein commented Jul 26, 2020 •

edited

Loading

krubenok commented Jul 26, 2020 •

edited

Loading