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

Feature/254 batch actions #600

Open
wants to merge 9 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion constants/routes.constant.js
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,15 @@ const searchRoutes = {
"get": {
requestType: Constants.REQUEST_TYPES.GET,
uri: "/api/search/"
}
},
"updateStatus": {
requestType: Constants.REQUEST_TYPES.GET,
uri: "/api/search/updateStatus",
Copy link
Member

Choose a reason for hiding this comment

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

can we match syntax with other routes?

},
"sendEmails": {
requestType: Constants.REQUEST_TYPES.GET,
uri: "/api/search/sendEmails",
Copy link
Member

Choose a reason for hiding this comment

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

can we match syntax with other routes?

},
};

const staffRoutes = {
Expand Down
2 changes: 2 additions & 0 deletions constants/success.constant.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ const HACKER_GET_BY_ID = "Hacker found by id.";
const HACKER_READ = "Hacker retrieval successful.";
const HACKER_CREATE = "Hacker creation successful.";
const HACKER_UPDATE = "Hacker update successful.";
const HACKER_UPDATE_EMAILS = "Hacker update emails sent."
const HACKER_SENT_WEEK_OF = "Hacker week-of email sent."
const HACKER_SENT_DAY_OF = "Hacker day-of email sent."

Expand Down Expand Up @@ -72,6 +73,7 @@ module.exports = {
HACKER_READ: HACKER_READ,
HACKER_CREATE: HACKER_CREATE,
HACKER_UPDATE: HACKER_UPDATE,
HACKER_UPDATE_EMAILS: HACKER_UPDATE_EMAILS,

HACKER_SENT_WEEK_OF: HACKER_SENT_WEEK_OF,
HACKER_SENT_DAY_OF: HACKER_SENT_DAY_OF,
Expand Down
19 changes: 18 additions & 1 deletion controllers/search.controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ const Services = {
};
const Util = require("../middlewares/util.middleware");
const Success = require("../constants/success.constant");
const ErrorMessages = require("../constants/error.constant")

async function searchResults(req, res) {
let results = req.body.results;
Expand All @@ -21,6 +22,22 @@ async function searchResults(req, res) {
});
}

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

Choose a reason for hiding this comment

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

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

message = Success.HACKER_UPDATE_EMAILS;
results = {}
} else {
message = ErrorMessages.EMAIL_500_MESSAGE;
}
return res.status(200).json({
message: message,
data: results
});
}

module.exports = {
searchResults: Util.asyncMiddleware(searchResults)
searchResults: Util.asyncMiddleware(searchResults),
emailResults: Util.asyncMiddleware(emailResults)
};
45 changes: 44 additions & 1 deletion middlewares/search.middleware.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ const Middleware = {
*/
function parseQuery(req, res, next) {
let query = req.body.q;

req.body.q = JSON.parse(query);

//Default page
Expand Down Expand Up @@ -62,6 +61,48 @@ async function executeQuery(req, res, next) {
return next();
}

/**
*
Copy link
Member

Choose a reason for hiding this comment

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

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

* @param {} req
Copy link
Member

Choose a reason for hiding this comment

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

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

* @param {*} res
* @param {*} next
*
* @returns
*/
async function executeStatusAction(req, res, next) {
req.body.results = await Services.Search.executeStatusAction(req.body.model,
req.body.q,
req.body.page,
req.body.limit,
req.body.sort,
req.body.sort_by,
req.body.update,
req.body.expand
);
return next();
}

/**
*
Copy link
Member

Choose a reason for hiding this comment

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

Ditto about the comment

* @param {} req
Copy link
Member

Choose a reason for hiding this comment

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

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

* @param {*} res
* @param {*} next
*
* @returns
*/
async function executeEmailAction(req, res, next) {
Copy link
Member

Choose a reason for hiding this comment

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

How do I trigger different types of emails? Is this restricted just to status emails?

Copy link
Member

Choose a reason for hiding this comment

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

Does the executeEmailAction service seem to only handle sending status updates? It's a little bit misleading right now. I would consider renaming it unless there are plans to have this function send emails for generic bulk actions that act on the result of a search. Or something like that.

req.body.results = await Services.Search.executeEmailAction(req.body.model,
req.body.q,
req.body.page,
req.body.limit,
req.body.sort,
req.body.sort_by,
req.body.status,
req.body.expand
);
return next();
}

function setExpandTrue(req, res, next) {
req.body.expand = true;
next();
Expand All @@ -71,5 +112,7 @@ function setExpandTrue(req, res, next) {
module.exports = {
parseQuery: parseQuery,
executeQuery: Middleware.Util.asyncMiddleware(executeQuery),
executeStatusAction: Middleware.Util.asyncMiddleware(executeStatusAction),
executeEmailAction: Middleware.Util.asyncMiddleware(executeEmailAction),
setExpandTrue: setExpandTrue,
};
20 changes: 20 additions & 0 deletions middlewares/validators/search.validator.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,4 +11,24 @@ module.exports = {
VALIDATOR.booleanValidator("query", "expand", true),
VALIDATOR.searchValidator("query", "q")
],
statusValidator: [
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

VALIDATOR.searchModelValidator("query", "model", false),
VALIDATOR.alphaValidator("query", "sort", true),
VALIDATOR.integerValidator("query", "page", true, 0),
VALIDATOR.integerValidator("query", "limit", true, 0, 1000),
VALIDATOR.searchSortValidator("query", "sort_by"),
VALIDATOR.booleanValidator("query", "expand", true),
VALIDATOR.searchValidator("query", "q"),
VALIDATOR.updateHackerValidator("query", "update")
],
emailValidator: [
Copy link
Member

Choose a reason for hiding this comment

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

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?

VALIDATOR.searchModelValidator("query", "model", false),
VALIDATOR.alphaValidator("query", "sort", true),
VALIDATOR.integerValidator("query", "page", true, 0),
VALIDATOR.integerValidator("query", "limit", true, 0, 1000),
VALIDATOR.searchSortValidator("query", "sort_by"),
VALIDATOR.booleanValidator("query", "expand", true),
VALIDATOR.searchValidator("query", "q"),
VALIDATOR.statusValidator("query", "status")
]
};
64 changes: 53 additions & 11 deletions middlewares/validators/validator.helper.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,8 @@ function integerValidator(fieldLocation, fieldname, optional = true, lowerBound

if (optional) {
return value.optional({
checkFalsy: true
})
checkFalsy: true
})
.isInt().withMessage(`${fieldname} must be an integer.`)
.custom((value) => {
return value >= lowerBound && value <= upperBound;
Expand Down Expand Up @@ -71,8 +71,8 @@ function mongoIdArrayValidator(fieldLocation, fieldname, optional = true) {

if (optional) {
return arr.optional({
checkFalsy: true
})
checkFalsy: true
})
.custom(isMongoIdArray).withMessage("Value must be an array of mongoIDs");
} else {
return arr.exists()
Expand Down Expand Up @@ -154,8 +154,8 @@ function regexValidator(fieldLocation, fieldname, optional = true, desire = Cons

if (optional) {
return match.optional({
checkFalsy: true
})
checkFalsy: true
})
.matches(desire)
.withMessage("must be valid url");
} else {
Expand Down Expand Up @@ -332,8 +332,8 @@ function jwtValidator(fieldLocation, fieldname, jwtSecret, optional = true) {
const jwtValidationChain = setProperValidationChainBuilder(fieldLocation, fieldname, "Must be vali jwt");
if (optional) {
return jwtValidationChain.optional({
checkFalsy: true
})
checkFalsy: true
})
.custom(value => {
const token = jwt.verify(value, jwtSecret);
if (typeof token !== "undefined") {
Expand Down Expand Up @@ -423,8 +423,8 @@ function searchValidator(fieldLocation, fieldname) {
function searchSortValidator(fieldLocation, fieldName) {
const searchSort = setProperValidationChainBuilder(fieldLocation, fieldName, "Invalid sort criteria")
return searchSort.optional({
checkFalsy: true
})
checkFalsy: true
})
.custom((value, {
req
}) => {
Expand Down Expand Up @@ -568,6 +568,46 @@ function enumValidator(fieldLocation, fieldname, enums, optional = true) {
}
}

/**
* Validates that the field is a valid hacker update object, and checks if corresponding new status is valid.
* @param {"query" | "body" | "header" | "param"} fieldLocation The location where the field should be found.
* @param {string} actionFieldName The name of the action that needs to be performed.
Copy link
Member

Choose a reason for hiding this comment

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

Update these comments

* @param {string} statusFieldName The name of the action that needs to be performed.
*/
function updateHackerValidator(fieldLocation, fieldName) {
const hackerObjectValue = setProperValidationChainBuilder(fieldLocation, fieldName, "Invalid hacker update object string.");

return hackerObjectValue.exists()
.withMessage("The hacker update object string must exist.")
.custom(updateHackerValidatorHelper).withMessage("The value must be a valid hacker update object.");
}

function updateHackerValidatorHelper(update) {
try {
var updateObject = JSON.parse(update);

if (updateObject && typeof updateObject === "object" && !("password" in updateObject)) {
Copy link
Member

Choose a reason for hiding this comment

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

Does the hacker schema have a password? If not, then we can remove the "password" part.

for (var key in updateObject) {
var schemaPath = Models.Hacker.searchableField(key);
if (!schemaPath) return false;
}
return true;
}
}
catch (e) {
return false;
}
}

function statusValidator(fieldLocation, statusFieldName) {
const statusValue = setProperValidationChainBuilder(fieldLocation, statusFieldName, "Invalid status.");
return statusValue.exists().withMessage("The status must exist!").custom((val, {
req
Copy link
Member

Choose a reason for hiding this comment

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

Does the { req } need to be in this function?

}) => {
return Constants.HACKER_STATUSES.includes(val);
}).withMessage("The value must be a proper status.")
}

/**
* Checks that 'value' is part of 'enums'. 'enums' should be an enum dict.
* @param {*} value Should be of the same type as the values of the enum
Expand Down Expand Up @@ -631,5 +671,7 @@ module.exports = {
dateValidator: dateValidator,
enumValidator: enumValidator,
routesValidator: routesValidator,
stringValidator: stringValidator
statusValidator: statusValidator,
stringValidator: stringValidator,
updateHackerValidator: updateHackerValidator,
};
91 changes: 91 additions & 0 deletions routes/api/search.js
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,97 @@ module.exports = {
Controllers.Search.searchResults
);

/**
* @api {get} /search/updateStatus execute an action on a specific query for any defined model
* @apiName search
Copy link
Member

Choose a reason for hiding this comment

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

Change the apiName to reflect the route.

* @apiGroup Search
* @apiVersion 0.0.8
Copy link
Member

Choose a reason for hiding this comment

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

Change this apiVersion number to the most recent version.

*
* @apiParam (query) {String} model the model to be searched
* @apiParam (query) {Array} q the query to be executed. For more information on how to format this, please see https://docs.mchacks.ca/architecture/
* @apiParam (query) {String} model the model to be searched
* @apiParam (query) {String} sort either "asc" or "desc"
* @apiParam (query) {number} page the page number that you would like
* @apiParam (query) {number} limit the maximum number of results that you would like returned
* @apiParam (query) {any} sort_by any parameter you want to sort the results by
* @apiParam (query) {String} json object string containing the update fields for the defined model
* @apiParam (query) {boolean} expand whether you want to expand sub documents within the results
Copy link
Member

Choose a reason for hiding this comment

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

Do we care about expanding the sub-documents for sending emails?

*
* @apiSuccess {String} message Success message
* @apiSuccess {Object} data Results
Copy link
Member

Choose a reason for hiding this comment

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

Do we send back the result of the search query? If not, then we should update this.

* @apiSuccessExample {object} Success-Response:
* {
"message": "Successfully executed query, returning all results",
Copy link
Member

Choose a reason for hiding this comment

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

Make sure the returned message here is correct.

"data": [
{...}
]
}
*
* @apiSuccess {String} message Success message
* @apiSuccess {Object} data Empty object
* @apiSuccessExample {object} Success-Response:
* {
"message": "No results found.",
"data": {}
}
*
* @apiError {String} message Error message
* @apiError {Object} data empty
* @apiErrorExample {object} Error-Response:
* {"message": "Validation failed", "data": {}}
*/
searchRouter.route("/updateStatus").get(
Copy link
Member

Choose a reason for hiding this comment

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

The route name doesn't feel quite right. This route can do much more than just update the status, no? It can update virtually any field of a hacker. Therefore, this route name should reflect that.

Middleware.Auth.ensureAuthenticated(),
Middleware.Auth.ensureAuthorized(),
Copy link
Member

Choose a reason for hiding this comment

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

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

Middleware.Validator.Search.statusValidator,
Middleware.parseBody.middleware,
Middleware.Search.parseQuery,
Middleware.Search.executeStatusAction,
Copy link
Member

Choose a reason for hiding this comment

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

If someone's status is updated, should we send an email immediately?

Controllers.Search.searchResults
);

/**
* @api {get} /search/sendEmails execute an action on a specific query for any defined model
* @apiName search
Copy link
Member

Choose a reason for hiding this comment

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

Change the apiName to reflect the route.

* @apiGroup Search
* @apiVersion 0.0.8
Copy link
Member

Choose a reason for hiding this comment

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

Ditto on apiVersion :)

*
* @apiParam (query) {String} model the model to be searched
* @apiParam (query) {Array} q the query to be executed. For more information on how to format this, please see https://docs.mchacks.ca/architecture/
* @apiParam (query) {String} model the model to be searched
* @apiParam (query) {String} sort either "asc" or "desc"
* @apiParam (query) {number} page the page number that you would like
* @apiParam (query) {number} limit the maximum number of results that you would like returned
* @apiParam (query) {any} sort_by any parameter you want to sort the results by
* @apiParam (query) {String} |
Copy link
Member

Choose a reason for hiding this comment

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

Please flesh this out.

* @apiParam (query) {boolean} expand whether you want to expand sub documents within the results
*
* @apiSuccess {String} message Success message
* @apiSuccess {Object} data Results
Copy link
Member

Choose a reason for hiding this comment

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

Ditto

* @apiSuccessExample {object} Success-Response:
* {
"message": "Hacker update emails sent.",
"data": {}
}
*
* @apiSuccess {String} message Error message
Copy link
Member

Choose a reason for hiding this comment

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

This would be @apiError

* @apiSuccess {Object} data Specific Error message
* @apiSuccessExample {object} Success-Response:
* {
"message": "Error while generating email",
"data": "Email service failed."
}
*/
searchRouter.route("/sendEmails").get(
Middleware.Auth.ensureAuthenticated(),
Middleware.Auth.ensureAuthorized(),
Copy link
Member

Choose a reason for hiding this comment

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

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

Middleware.Validator.Search.emailValidator,
Middleware.parseBody.middleware,
Middleware.Search.parseQuery,
Middleware.Search.executeEmailAction,
Copy link
Member

Choose a reason for hiding this comment

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

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?

Controllers.Search.emailResults
);

apiRouter.use("/search", searchRouter);
}
};
Loading