#259 patch remove section from issue map - #298
Conversation
Added the addArticle and removeSection accessors. Added the preliminary structure for tests.
Changed seeding data for specific tests. Made sure articles added to general article list is unique.
Further changed the reseed data such that the general and articles in sections are unique. Changed corresponding tests.
| * @param {Request} req | ||
| * @param {Response} res | ||
| */ | ||
| static async patchSection(req, res) { |
There was a problem hiding this comment.
Given that this function only removes sections, I would rename the function to patchRemoveSection.
| const sectionColor = req.body.sectionColor; | ||
|
|
||
| if (!sectionName || !sectionColor) { | ||
| throw new ErrorInvalidRequestBody(); |
There was a problem hiding this comment.
Given the number of possibilities for an invalid request body, a specific message may help with specificity.
| const sectionName = req.body.sectionName; | ||
| const sectionColor = req.body.sectionColor; | ||
|
|
||
| if (!sectionName || !sectionColor) { |
There was a problem hiding this comment.
Also check for the issue number being present? If you don't have an issue number, where are you deleting the section from?
| await updated.save(); | ||
|
|
||
| // remove section | ||
| const issues = await IssueMap.findOneAndUpdate( |
There was a problem hiding this comment.
Nitpick: If you're only updating one issue, why is this const pluralized
| { issueNumber: issueNumber }, | ||
| { | ||
| $pull: { | ||
| sections: { |
There was a problem hiding this comment.
You use $elemMatch below, why not here? Otherwise (as I understand it) you aren't ensuring that both portions match.
There was a problem hiding this comment.
$pull will delete elements that satisfy both requirements.
| } | ||
|
|
||
| // extract articles | ||
| const toMove = article.sections[0].articles; |
There was a problem hiding this comment.
Related to my above issue - why does the index to 0 just work? How do you know that the section is going to be the first one in the array?
| * @return issue map with added articles to general field | ||
| */ | ||
| static async addArticles(issueNumber, sectionName, sectionColor) { | ||
| const article = await IssueMap.findOne( |
There was a problem hiding this comment.
So, this is not an article? It seems that the return of this function is an IssueMap.
|
|
||
| if (!finalArticle) { | ||
| throw new ErrorIssueMapNotFound(); | ||
| } |
There was a problem hiding this comment.
I would move this check above, because you've already looked for this issueMap once. If you couldn't find it then, the error should have been thrown then.
There was a problem hiding this comment.
This is to ensure nothing weird happens when articles are added.
| sectionName: "Project Discussion", | ||
| sectionColor: "#FF5733", | ||
| }; | ||
|
|
There was a problem hiding this comment.
This section doesn't have multiple articles
| }) | ||
| ); | ||
|
|
||
| expect(response.body.articles).toStrictEqual(validSection.articles); |
There was a problem hiding this comment.
If you're only doing article array comparisons, you don't really need a full object, no?
Fixed testing data and PR comments.
2 participants
Pull Request
Brief Summary
Questions / Considerations for the Future
API Changes
Database Changes
-
New Tests
Closes #259