-
Notifications
You must be signed in to change notification settings - Fork 5
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
Fix ability to move merch item under another collection #432
Conversation
Thanks for contributing! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one quick question
@@ -346,7 +346,9 @@ export default class MerchStoreService { | |||
.merchStoreCollection(txn) | |||
.findByUuid(updatedCollection); | |||
if (!collection) throw new NotFoundError('Merch collection not found'); | |||
updatedItem.collection = collection; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you look at line 341, there's a call to MerchandiseItemModel.merge()
. However, line 352 (merchItemRepository.upsertMerchItem()
) calls MerchandiseItemModel.merge()
internally. What's the reason for this difference? Is there a need to call the model's merge method directly prior to checking if there's an updated collection?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we merge at line 341 so that we can immediately test whether the merged item is a valid item or not on line 342, although upsertMerchItem does call merge internally, it does not do the checking by itself and requires external check for validations. In addition, merge is a fast operation so we can kinda use it freely.
closing this bc @nik-dange addressed it in his pr |
Info
Closes #433.
Description
What changes did you make? List all distinct problems that this PR addresses. Explain any relevant
motivation or context.
enable collection property to be editable through PATCH /item/:uuid
Changes
edited the logic for merch store service over itemedit
Type of Change
expected)
workflows, linting, etc.)
If you've selected Patch, Minor, or Major as your change type, make sure to bump the version before merging in
package.json
!Testing
I have tested that my changes fully resolve the linked issue ...
Checklist
package.json
file.Screenshots
Please include a screenshot of your Postman testing passing successfully.