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

Fix Assortment Paths #385

Merged
merged 6 commits into from
Oct 13, 2021
Merged

Fix Assortment Paths #385

merged 6 commits into from
Oct 13, 2021

Conversation

pozylon
Copy link
Member

@pozylon pozylon commented Aug 13, 2021

…ssortments.findOne

@pozylon
Copy link
Member Author

pozylon commented Oct 12, 2021

Issues with consistency:

  • createAssortmentLink and createAssortmentProduct do not check if the provided childAssortmentId or productId exist
  • the helpers addLink and addProduct (+ addFilter, ...) on assortment call createAssortmentLink/createAssortmentProduct respectively, also without checking if provided data results in failed links
  • the mutation that call addLink, addProduct and addFilter ARE CHECKING THAT IT'S VALID
  • the bulk importer DOES NOT check if data is valid so a wrong bulk import event could lead to crap data in the db
  • Filters.removeFilter does not remove AssortmentFilters but completely removed Filters from the db

This could lead to broken breadcrumbs and exceptions because the GraphQL Schema state that assortmentProduct on ProductAssortmentPath and assortmentId/assortmentTexts are NON-NULL and AssortmentLink, AssortmentFilter and AssortmentProduct state that the edges are NON-NULL

How to solve this?

a) Fix on edit

  • We add existence checks to the bulk importer
  • Add a cleanup migration
  • Fix removing filters does not remove AssortmentFilters
  • Fix removing products does not remove AssortmentProducts

b) Relax the graphql schema

  • Make all NON-NULL references of products, assortments and filters NULLABLE

Going with B is like sending the rotten tomato to the client, shall the client deal with the mess. We should always try to keep the DB as consistent as possible so we should do A.

@pozylon pozylon changed the title Add new assortment link breadcrumb resolver that saves n+1 calls to A… Fix Assortment Paths Oct 12, 2021
Copy link
Contributor

@Mikearaya Mikearaya left a comment

Choose a reason for hiding this comment

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

looks good to me 👍

@pozylon pozylon merged commit b2b8963 into master Oct 13, 2021
@pozylon pozylon deleted the improve-assortment-links branch October 22, 2021 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants