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

Changes to products API #253

Merged
merged 7 commits into from
Feb 10, 2024
Merged

Changes to products API #253

merged 7 commits into from
Feb 10, 2024

Conversation

A-Guldborg
Copy link
Contributor

This PR moves MenuItems into their own directories instead of residing amongst the products.

Furthermore, this PR removes the ChangedProductResponse and replaces this with the ProductResponse. The ChangedProductResponse was used as the return type for products that was added or updated. Instead of having multiple DTOs which would each have to be updated whenever a change was made to the general entity, we now only have one.
The previous implementation of ChangedProductResponse did not contain the value IsPerk nor the Id. This meant that adding a product in shifty would not be able to show the new item's ID (nor conduct further changes) without reloading the whole table of products (i.e. a get request to the /products/all endpoint).
If there was any particular reason to keep these divided, please let me know @HubertWojcik10 :-)

@A-Guldborg A-Guldborg added bug Verified bug api-v2 Related to new API version 2 labels Jan 25, 2024
@A-Guldborg A-Guldborg self-assigned this Jan 25, 2024
@ghost
Copy link

ghost commented Jan 25, 2024

👇 Click on the image for a new way to code review

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map legend

jonasanker
jonasanker previously approved these changes Jan 27, 2024
Copy link
Member

@jonasanker jonasanker left a comment

Choose a reason for hiding this comment

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

I'm okay with this PR.

@A-Guldborg I'm always discussing with myself whether it should be the controller layer or the service/repository layer which should be responsible for mapping the object. In API v2, I have generally avoided having a mapper method on the controller layer but I'm unsure whether this should be enforced, thus you get the approval with suggestions :)

@A-Guldborg
Copy link
Contributor Author

@jonasanker as discussed, I have moved the mapper to the product layer. Please be vigilant when reviewing these extra changes; I do not have the knowledge to be 100% sure that this does not break anything in the purchase flow (using ProductResponse in the PurchaseController rather than the actual Product entity in CheckUserIsAllowedToPurchaseProduct and InitiatePaymentAsync)

@A-Guldborg A-Guldborg requested a review from jonasanker February 8, 2024 18:31
Copy link

sonarqubecloud bot commented Feb 8, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
73.2% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

@jonasanker jonasanker merged commit 84849d2 into main Feb 10, 2024
5 of 6 checks passed
@jonasanker jonasanker deleted the bug/changes-to-products-api branch February 10, 2024 07:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-v2 Related to new API version 2 bug Verified bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants