Feature/254 batch actions #600

printharsh · 2019-11-03T20:01:36Z

Description

Allows batch actions (hacker status updates and sending emails) for a search query provided.

Currently, updateStatus only works for hacker models, change name to updateHackerStatuses and sendEmailsToHackers?

Fixes #254 #276

Type of change

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

How Has This Been Tested?

Tested manually with queries of hackers.

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
I have made corresponding changes to the documentation
My changes generate no new warnings
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
Any dependent changes have been merged and published in downstream modules

Merge into Master

pierreTklein

Cool work! Check my comments for fixes.

pierreTklein · 2019-11-03T23:22:14Z

controllers/search.controller.js

+async function emailResults(req, res) {
+    let results = req.body.results;
+    let message;
+    if (results === undefined) {


Error-handling should be done via next(<err message>);. See this example for reference.
This way, *.controller.js only handles successes.

pierreTklein · 2019-11-03T23:25:14Z

middlewares/search.middleware.js

@@ -62,6 +61,48 @@ async function executeQuery(req, res, next) {
    return next();
 }

+/**
+ * 


Make sure to flesh out this comment so we know what this function does :)!

pierreTklein · 2019-11-03T23:25:27Z

middlewares/search.middleware.js

+}
+
+/**
+ * 


Ditto about the comment

pierreTklein · 2019-11-03T23:26:01Z

middlewares/search.middleware.js

@@ -62,6 +61,48 @@ async function executeQuery(req, res, next) {
    return next();
 }

+/**
+ * 
+ * @param {} req 


Consider adding what attributes this function expects to be inside of req.

pierreTklein · 2019-11-03T23:26:17Z

middlewares/search.middleware.js

+
+/**
+ * 
+ * @param {} req 


Consider adding what attributes this function expects to be inside of req.

pierreTklein · 2019-11-03T23:39:57Z

routes/api/search.js

+                    "data": {}
+                }
+         *
+         * @apiSuccess {String} message Error message


This would be @apiError

services/search.service.js

pierreTklein · 2019-11-03T23:45:26Z

services/search.service.js

-        query.sort(sort_by);
+        query.sort(sortBy);
+    }
+    return query.limit(limit).skip(limit * page)


What happens if limit / page are undefined?

pierreTklein · 2019-11-03T23:47:12Z

services/search.service.js

+ * @returns {Promise<[Array]>}
+ * @description Builds and executes a status update based on a subset of mongodb
+ */
+function executeStatusAction(model, queryArray, page, limit, sort, sortBy, update, shouldExpand = false) {


This method is more generic than executeStatus. This is a batch update for any model based on a query and a model. Therefore, the method name should reflect that.

pierreTklein · 2019-11-03T23:49:58Z

services/search.service.js

+ * @description Sends a status update email based on a subset of mongodb
+ */
+async function executeEmailAction(model, queryArray, page, limit, sort, sortBy, status, shouldExpand = false) {
+    var query = createQuery(model, queryArray, page, limit, sort, sortBy, shouldExpand);


This logic should not necessarily be in the executeEmailAction method, or at least if it is, then the parameters should reflect some sort of restrictions. For example, the code below assumes that the model is of type Hacker. However, the method parameters take in a model object!

I would recommend this function be used only for Hacker objects, since it send a status update email. Because of this, the function name should change to reflect this.

pierreTklein · 2019-11-04T00:19:36Z

This requires a documentation change, so run npm run docs or something of that sort.
Make sure to write a few test calls to your API!

YiFeiZhang2 · 2019-11-05T04:44:15Z

middlewares/validators/search.validator.js

@@ -11,4 +11,24 @@ module.exports = {
        VALIDATOR.booleanValidator("query", "expand", true),
        VALIDATOR.searchValidator("query", "q")
    ],
+    statusValidator: [


Does this validator validate for status? It seems not necessarily a search and update status validator, but rather a search and update hacker validator?

Also, note that you can chain validator middlewares in the route as well! Consider that both your new validators perform the function of 'check for valid search' + 'check for something else', and searchQueryValidator already performs the first part of that. Instead of having your validators duplicate this code, you could chain searchQueryValidator as well as statusValidator in the route.

YiFeiZhang2 · 2019-11-05T04:51:33Z

middlewares/validators/search.validator.js

+        VALIDATOR.searchValidator("query", "q"),
+        VALIDATOR.updateHackerValidator("query", "update")
+    ],
+    emailValidator: [


Same notes as above, but also, it's kinda weird that the emailValidator checks the status? I see this is being used in the bulk email endpoint, but should the name of a thing describe what it does, or where it's being used? What kind of coupling does that cause?

YiFeiZhang2 · 2019-11-05T04:58:58Z

routes/api/search.js

+            Middleware.Validator.Search.emailValidator,
+            Middleware.parseBody.middleware,
+            Middleware.Search.parseQuery,
+            Middleware.Search.executeEmailAction,


This endpoint is interesting. The name sounds like a generic 'send emails' route. The middleware names look like generic 'send emails' validators. But 'emailValidator' validates for 'search + status', and then the executeEmailAction middleware ends up calling a service that seems to only send status update emails. I believe there's a mismatch between what this endpoint says it's doing, and what it actually does.

Are there plans to expand the functionality of the route? I think aligning functionality and naming would help. Also, if this is solely a status email sender, consider (from a comment above):
[...,
Middleware.Validator.Search.searchQueryValidator,
Middleware.Validator.Search.statusValidator,
...]

Does that align more or less with what this endpoint currently does?

YiFeiZhang2

Nice work! My comments really deal with maintainability rather than functionality.

I think the biggest thing for me was a bit of confusion between the names of things and what they actually do.

pierreTklein · 2019-11-05T13:20:39Z

routes/api/search.js

+         */
+        searchRouter.route("/updateStatus").get(
+            Middleware.Auth.ensureAuthenticated(),
+            Middleware.Auth.ensureAuthorized(),


Make sure only admins / staff are able to change this status!

pierreTklein · 2019-11-05T13:21:10Z

routes/api/search.js

+         */
+        searchRouter.route("/sendEmails").get(
+            Middleware.Auth.ensureAuthenticated(),
+            Middleware.Auth.ensureAuthorized(),


Make sure only admins / staff are able to send emails too

loreina · 2019-11-17T17:52:31Z

constants/routes.constant.js

+    },
+    "updateStatus": {
+        requestType: Constants.REQUEST_TYPES.GET,
+        uri: "/api/search/updateStatus",


can we match syntax with other routes?

loreina · 2019-11-17T17:52:38Z

constants/routes.constant.js

+    },
+    "sendEmails": {
+        requestType: Constants.REQUEST_TYPES.GET,
+        uri: "/api/search/sendEmails",


can we match syntax with other routes?

pierreTklein · 2019-12-04T03:01:29Z

Any updates on this PR?

loreina · 2019-12-05T22:13:16Z

@pierreTklein backlogged for now

YiFeiZhang2 and others added 8 commits February 1, 2019 22:18

Merge pull request #329 from hackmcgill/develop

6982db6

Merge into Master

version 1.6.1

04932d1

Works for batch status updates

1bee227

Constants + ToDo, email on local

d8b634a

Finished email

b135286

Formatting changes

c6c8234

Changed comments

32d7eca

Removed unnecessary functions added

be46e7a

printharsh requested review from pierreTklein, loreina and Ty-Won November 3, 2019 21:25

Merge branch 'develop' into feature/254-batchActions

6fda21e

pierreTklein requested changes Nov 3, 2019

View reviewed changes

pierreTklein requested a review from YiFeiZhang2 November 3, 2019 23:50

YiFeiZhang2 reviewed Nov 5, 2019

View reviewed changes

pierreTklein reviewed Nov 13, 2019

View reviewed changes

loreina reviewed Nov 17, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/254 batch actions #600

Feature/254 batch actions #600

printharsh commented Nov 3, 2019 •

edited

Loading

pierreTklein left a comment

pierreTklein Nov 3, 2019

pierreTklein Nov 3, 2019

pierreTklein Nov 3, 2019

pierreTklein Nov 3, 2019

pierreTklein Nov 3, 2019

pierreTklein Nov 3, 2019

pierreTklein Nov 3, 2019

pierreTklein Nov 3, 2019

pierreTklein Nov 3, 2019

pierreTklein commented Nov 4, 2019

YiFeiZhang2 Nov 5, 2019

YiFeiZhang2 Nov 5, 2019

YiFeiZhang2 Nov 5, 2019

YiFeiZhang2 Nov 5, 2019

YiFeiZhang2 left a comment

pierreTklein Nov 5, 2019

pierreTklein Nov 5, 2019

loreina Nov 17, 2019

loreina Nov 17, 2019

pierreTklein commented Dec 4, 2019

loreina commented Dec 5, 2019

+              }
+              /**
+               *

Feature/254 batch actions #600

Are you sure you want to change the base?

Feature/254 batch actions #600

Conversation

printharsh commented Nov 3, 2019 • edited Loading

Description

Type of change

How Has This Been Tested?

Checklist:

pierreTklein left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pierreTklein commented Nov 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YiFeiZhang2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pierreTklein commented Dec 4, 2019

loreina commented Dec 5, 2019

printharsh commented Nov 3, 2019 •

edited

Loading