Feature/254 batch actions by printharsh · Pull Request #600 · hackmcgill/hackerAPI · GitHub
Skip to content
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
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Loading