Feature/153 accept hacker by email #620

ManethKulatunge · 2019-12-15T03:23:00Z

Tickets:

HCK-153

List of changes:

-Added a route to accept hacker by email
-Made changes to middleware to allow aforementioned 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

Postman request to hacker/acceptEmail/ with email as param
Hacker.Test tests

Test Configuration:

Firmware version:
Hardware:
Toolchain:
SDK:

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

pierreTklein · 2019-12-15T05:38:53Z

middlewares/hacker.middleware.js

+/**
+ * Updates a hacker that is specified by req.params.email, and then sets req.email
+ * to the email of the hacker, found in Account.
+ * @param {{params:{id: string}, body: *}} req


Is the attribute of params id, or _id? Make sure to verify this

pierreTklein · 2019-12-15T05:40:05Z

middlewares/hacker.middleware.js

+ * @param {*} next
+ */
+async function updateHackerByEmail(req, res, next) {
+    const hacker = await Services.Hacker.updateOne(req.body.hacker._id, req.body);


We’re updating the hacker in this method by ID, so why is it called “update hacker by Email”?

I think in this case you're not updating, you're just using the hacker to search for an account via ID so you can get the email, so I think you should change the middleware name and change the service method you call

printharsh · 2019-12-15T05:43:50Z

middlewares/hacker.middleware.js

+ * @param {*} next 
+ */
+function parseAcceptEmail(req, res, next) {
+    req.body.hacker.status = Constants.General.HACKER_STATUS_ACCEPTED;


Is there a reason this is req.body.hacker.status, i think the update middleware uses req.body.status right?

agreed, the comment says req.body.status ^^ Double check it unless you changed it.

printharsh · 2019-12-15T05:45:40Z

routes/api/hacker.js

+
+        /**
+         * @api {patch} /hacker/accept/:email accept a Hacker
+         * @apiName acceptHacker


Nit: update to be for accept by email

printharsh · 2019-12-15T05:46:58Z

tests/hacker.test.js


+    it("should FAIL to accept a hacker on /api/hacker/acceptEmail/:email due to authentication", function(done) {
+        chai.request(server.app)
+            .patch(`/api/hacker/acceptEmail/${TeamHacker0.email}`)


Believe it should be the account's email as emails are tied to accounts and not hackers.

printharsh · 2019-12-15T05:48:09Z

tests/util/hacker.test.util.js


 const TeamHacker0 = {
    _id: Constants.MongoId.hackerAId,
+    email: "hello@gmail.com",


Emails are tied to accounts not hackers.

Ty-Won · 2019-12-22T23:43:52Z

routes/api/hacker.js

+            Middleware.Auth.ensureAuthorized([Services.Hacker.findByEmail]),
+            Middleware.Validator.RouteParam.emailValidator,
+            Middleware.parseBody.middleware,
+            Middleware.Hacker.findByEmail,


is middleware hacker findByEmail and then calling middleware findByEmail duplicate work?

Yeah I believe so I'll fix it

Edit: req.body.hacker is changed by findByEmail so no it is not duplicated work

Ty-Won · 2019-12-22T23:43:59Z

middlewares/hacker.middleware.js

+ * @param {*} next 
+ */
+function parseAcceptEmail(req, res, next) {
+    req.body.hacker.status = Constants.General.HACKER_STATUS_ACCEPTED;


agreed, the comment says req.body.status ^^ Double check it unless you changed it.

Ty-Won · 2020-01-12T18:47:49Z

middlewares/hacker.middleware.js

+ * @param {*} next
+ */
+async function updateHackerByEmail(req, res, next) {
+    const hacker = await Services.Hacker.updateOne(req.body.hacker._id, req.body);


I think in this case you're not updating, you're just using the hacker to search for an account via ID so you can get the email, so I think you should change the middleware name and change the service method you call

ManethKulatunge requested review from loreina and printharsh December 15, 2019 03:23

pierreTklein reviewed Dec 15, 2019

View reviewed changes

printharsh reviewed Dec 15, 2019

View reviewed changes

ManethKulatunge force-pushed the feature/153-accept-hacker-email branch 2 times, most recently from f54f3a2 to b17bada Compare December 16, 2019 00:39

ManethKulatunge requested a review from Ty-Won December 22, 2019 18:42

Ty-Won reviewed Dec 22, 2019

View reviewed changes

ManethKulatunge force-pushed the feature/153-accept-hacker-email branch from 1c711ad to f067f6e Compare December 23, 2019 02:52

ManethKulatunge requested a review from Ty-Won December 30, 2019 05:55

ManethKulatunge force-pushed the feature/153-accept-hacker-email branch from f067f6e to 98afcf7 Compare December 30, 2019 08:13

ManethKulatunge added 7 commits January 12, 2020 11:11

Route created

07e6081

Middleware made for the route

e7c92f6

Tests(need to be edited)

10b35ae

Edited route

e570335

status email middleware made

91e1082

Edited tests

efce4fc

Fixed issues with referenced accounts

61fd988

ManethKulatunge force-pushed the feature/153-accept-hacker-email branch from 8ef860c to 61fd988 Compare January 12, 2020 16:11

ManethKulatunge added 3 commits January 12, 2020 13:50

edited service used for obtaining hacker email

412a116

Comment changed for parseAcceptEmail

75003a5

edited route description

4e86599

Ty-Won approved these changes Jan 12, 2020

View reviewed changes

Ty-Won merged commit 90279b9 into develop Jan 12, 2020

Ty-Won deleted the feature/153-accept-hacker-email branch January 12, 2020 20:16

Feature/153 accept hacker by email #620

Feature/153 accept hacker by email #620

Uh oh!

Conversation

ManethKulatunge commented Dec 15, 2019 • edited 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ManethKulatunge commented Dec 15, 2019 •

edited

Loading