Walter/card patch delete #59

waltervan00 · 2021-08-31T03:44:20Z

Hi all. I have finished writing unit tests for PATCH and DELETE endpoints.

Please confirm that the tests are valid.
For PATCH, please confirm that the image (optional field for cards) can be removed by specifying them as null.

Closes #25.
Closes #26.

… into walter/cardPatchDelete

…ellyPro into walter/cardPatchDelete

… into walter/cardPatchDelete

…ECT-PorkBellyPro into walter/cardPatchDelete

waltervan00 · 2021-08-31T03:51:52Z

Appears to be a problem with the linter resolving the path to the shared folder. Linting locally passes.

server/src/__tests__/api/authenticated/cardDelete.ts

chomosuke

overall pretty good, some minor changes needed.

server/src/api/authenticated/cardPatch.ts

server/src/api/authenticated/router.ts

server/src/__tests__/api/authenticated/cardDelete.ts

server/src/api/authenticated/cardDelete.ts

server/src/api/authenticated/cardPatch.ts

chomosuke

lg

chomosuke

last bit of stuff

server/src/api/authenticated/router.ts

shangzhel

There are better ways to do some of these.

shangzhel · 2021-08-31T13:36:21Z

server/src/api/authenticated/cardDelete.ts

+    try {
+      await dbs.withTransaction(async () => {
+        // removes card in collection belonging to user
+        const cardToDelete = await this.parent.Cards.findById(id);


Use the ObjectId object constructed earlier instead of giving the string id here.

shangzhel · 2021-08-31T13:40:01Z

server/src/api/authenticated/cardDelete.ts

+        const cardToDelete = await this.parent.Cards.findById(id);
+        if (!cardToDelete) {
+          throw new HttpStatusError(410);
+        } else if (user.id !== cardToDelete.user.toString()) {


Directly compare the user ObjectIds with ObjectId.prototype.equals(other).

Suggested change

} else if (user.id !== cardToDelete.user.toString()) {

} else if (!user._id.equals(cardToDelete.user)) {

Reference: https://github.com/mongodb/js-bson/blob/6dcd46e677c40c4409d6f601aaf57ce15d4d05d1/src/objectid.ts#L212

Throws an error with linter. Error is dangling underscore. Note that user.id gives a string type of id.

shangzhel · 2021-08-31T13:46:03Z

server/src/api/authenticated/cardDelete.ts

+        } else if (user.id !== cardToDelete.user.toString()) {
+          throw new HttpStatusError(401);
+        } else {
+          await this.parent.Cards.findByIdAndDelete(id);


Use Model.prototype.remove() to delete a document that you had already found instead of finding it again.

Suggested change

await this.parent.Cards.findByIdAndDelete(id);

await cardToDelete.remove();

Reference: https://mongoosejs.com/docs/api/model.html#model_Model-remove

shangzhel · 2021-09-01T12:35:59Z

server/src/api/authenticated/cardPatch.ts

+        // if card is not found
+        if (!updatedCard) {
+          throw new HttpStatusError(404);
+        } else if (updatedCard.user.toString() !== user.id) {


Directly compare ObjectIds with equals()

Suggested change

} else if (updatedCard.user.toString() !== user.id) {

} else if (!updatedCard.user.equals(user._id)) {

shangzhel · 2021-09-01T12:36:41Z

server/src/api/authenticated/cardPatch.ts

+              const tag = await this.parent.Tags.findById(tagId);
+              if (tag == null) {
+                throw new HttpStatusError(400);
+              } else if (tag.user.toString() !== user.id) {


Directly compare ObjectIds with equals()

Suggested change

} else if (tag.user.toString() !== user.id) {

} else if (!tag.user.equals(user._id)) {

…ObjectId

shangzhel

Not quite right yet.

shangzhel · 2021-09-02T03:56:36Z

server/src/api/authenticated/cardDelete.ts

        if (!cardToDelete) {
          throw new HttpStatusError(410);
-        } else if (user.id !== cardToDelete.user.toString()) {
+        } else if (!user.id.equals(cardToDelete.user)) {


Wrong property. id is the string representation of the document id. _id is the actual mongoose.Types.ObjectId.

Suggested change

} else if (!user.id.equals(cardToDelete.user)) {

} else if (!user._id.equals(cardToDelete.user)) {

Accessing the _id property is valid. However the linter will produce an error and prevent this code from passing.

shangzhel · 2021-09-02T03:57:13Z

server/src/api/authenticated/cardPatch.ts

              const tag = await this.parent.Tags.findById(tagId);
              if (tag == null) {
                throw new HttpStatusError(400);
-              } else if (tag.user.toString() !== user.id) {
+              } else if (!user.id.equals(tag.user)) {


Same as above, wrong property.

Suggested change

} else if (!user.id.equals(tag.user)) {

} else if (!user._id.equals(tag.user)) {

shangzhel · 2021-09-02T03:57:44Z

server/src/api/authenticated/cardPatch.ts

        // if card is not found
        if (!updatedCard) {
          throw new HttpStatusError(404);
-        } else if (updatedCard.user.toString() !== user.id) {
+        } else if (!user.id.equals(updatedCard.user)) {


Same as above, wrong property.

Suggested change

} else if (!user.id.equals(updatedCard.user)) {

} else if (!user._id.equals(updatedCard.user)) {

shangzhel · 2021-09-02T03:58:09Z

server/src/api/authenticated/cardPatch.ts

          throw new HttpStatusError(401);
        } else {
          updatedCard.set(updateDetails);
          await updatedCard.save();
        }

        const response: CardPatchResponse = {
-          id: updatedCard.id,
+          id: updatedCard.id.toString(),


Change unnecessary, id is already a string.

shangzhel

Looks good to me.

waltervan00 added 11 commits August 29, 2021 11:42

cardPatch endpoint

a77c5f5

changed patch logic to exclude updating undefined field

3017299

implemented Delete endpoint

d5b5448

minor fixes to cardPatch

8cf2676

removed id from updateDetails in PATCH

b5b944d

Merge branch 'master' of github.com:chomosuke/IT-PROJECT-PorkBellyPro…

efa1b08

… into walter/cardPatchDelete

Merge branch 'richard/image' of github.com:chomosuke/IT-PROJECT-PorkB…

d53b7aa

…ellyPro into walter/cardPatchDelete

Merge branch 'master' of github.com:chomosuke/IT-PROJECT-PorkBellyPro…

55964fb

… into walter/cardPatchDelete

added unit tests for card PATCH

bcca653

fixed PATCH to properly reset existing cards state

2f19a6b

added unit tests for DELETE card

c9ed9a1

waltervan00 requested review from chomosuke and shangzhel August 31, 2021 03:44

waltervan00 added 2 commits August 31, 2021 13:45

added unit tests for DELETE card

dd459b5

Merge branch 'walter/cardPatchDelete' of github.com:chomosuke/IT-PROJ…

e1852fe

…ECT-PorkBellyPro into walter/cardPatchDelete

chomosuke requested changes Aug 31, 2021

View reviewed changes

server/src/__tests__/api/authenticated/cardDelete.ts Outdated Show resolved Hide resolved

changed CardDeleteRequest import path

493a55e

chomosuke self-requested a review August 31, 2021 04:01

chomosuke requested changes Aug 31, 2021

View reviewed changes

changes to check string id validity

e90b283

chomosuke assigned waltervan00 Aug 31, 2021

chomosuke added the enhancement New feature or request label Aug 31, 2021

chomosuke added this to the Sprint 1 milestone Aug 31, 2021

waltervan00 requested a review from chomosuke August 31, 2021 13:18

chomosuke approved these changes Aug 31, 2021

View reviewed changes

chomosuke self-requested a review August 31, 2021 13:38

chomosuke requested changes Aug 31, 2021

View reviewed changes

server/src/api/authenticated/router.ts Outdated Show resolved Hide resolved

reduced delete json limit to default value

654af8a

chomosuke approved these changes Aug 31, 2021

View reviewed changes

shangzhel requested changes Sep 1, 2021

View reviewed changes

minor changes to cardPatch and Delete to check identity through user …

d363c69

…ObjectId

waltervan00 requested a review from shangzhel September 2, 2021 01:53

shangzhel requested changes Sep 2, 2021

View reviewed changes

foo

a425e6d

waltervan00 force-pushed the walter/cardPatchDelete branch from d363c69 to 654af8a Compare September 2, 2021 05:40

chomosuke assigned chomosuke and unassigned chomosuke Sep 2, 2021

using _id.equality for PATCH and DELETE endpoints

c5ee6a0

shangzhel approved these changes Sep 2, 2021

View reviewed changes

waltervan00 merged commit 2dda106 into master Sep 2, 2021

waltervan00 deleted the walter/cardPatchDelete branch September 2, 2021 08:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Walter/card patch delete #59

Walter/card patch delete #59

waltervan00 commented Aug 31, 2021 •

edited by chomosuke

Loading

waltervan00 commented Aug 31, 2021

chomosuke left a comment

chomosuke left a comment

chomosuke left a comment

shangzhel left a comment

shangzhel Aug 31, 2021

shangzhel Aug 31, 2021

waltervan00 Sep 1, 2021 •

edited

Loading

shangzhel Aug 31, 2021

shangzhel Sep 1, 2021

shangzhel Sep 1, 2021

shangzhel left a comment

shangzhel Sep 2, 2021

waltervan00 Sep 2, 2021

shangzhel Sep 2, 2021

shangzhel Sep 2, 2021

shangzhel Sep 2, 2021

shangzhel left a comment

	} else if (user.id !== cardToDelete.user.toString()) {
	} else if (!user._id.equals(cardToDelete.user)) {

	await this.parent.Cards.findByIdAndDelete(id);
	await cardToDelete.remove();

	} else if (updatedCard.user.toString() !== user.id) {
	} else if (!updatedCard.user.equals(user._id)) {

	} else if (tag.user.toString() !== user.id) {
	} else if (!tag.user.equals(user._id)) {

	} else if (!user.id.equals(cardToDelete.user)) {
	} else if (!user._id.equals(cardToDelete.user)) {

	} else if (!user.id.equals(tag.user)) {
	} else if (!user._id.equals(tag.user)) {

	} else if (!user.id.equals(updatedCard.user)) {
	} else if (!user._id.equals(updatedCard.user)) {

Walter/card patch delete #59

Walter/card patch delete #59

Conversation

waltervan00 commented Aug 31, 2021 • edited by chomosuke Loading

waltervan00 commented Aug 31, 2021

chomosuke left a comment

Choose a reason for hiding this comment

chomosuke left a comment

Choose a reason for hiding this comment

chomosuke left a comment

Choose a reason for hiding this comment

shangzhel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

waltervan00 Sep 1, 2021 • edited Loading

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

shangzhel 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

shangzhel left a comment

Choose a reason for hiding this comment

waltervan00 commented Aug 31, 2021 •

edited by chomosuke

Loading

waltervan00 Sep 1, 2021 •

edited

Loading