OptionTypes loaded from DB instead of off product #43
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
derrickp
force-pushed
the
option-types-saving
branch
4 times, most recently
from
bcf38eb
to
82f8a94
Compare
MinasMazar
suggested changes
Nov 13, 2020
added 3 commits
November 13, 2020 09:45
We force the name to be downcase when we make the option type inside of the code, so I think it makes sense to make these match up with what we're doing in the code.
This covers the case where the option types already exist inside of the DB, but do not exist on the product that is being imported. In this scenario, we should not be saving new option types, but with the current code it does. I'm marking the spec as pending because it will fail, but will be passing in the next commit.
In the case where the option types already exist in the DB, but not in the product, we should look for them there first, so that we can use the ones we've already got instead of making more. This also adds some code so we're not overriding any information already present on the option type, specifically the presentation field, as we cannot really assume that the presentation should be equal to the name.
derrickp
force-pushed
the
option-types-saving
branch
from
82f8a94
to
8494a39
Compare
|
@MinasMazar I think I've addressed your comments. Thanks for the input! |
MinasMazar
approved these changes
Nov 13, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While I was importing some products into a store I was working on I noticed that I seemed to be getting duplicate Spree::OptionType values in the database, and I believe it was coming from the OptionType import here. I was able to replicate the issue by first saving the option type in the specs to the database and ensure they weren't on the product.
I think this is a use case that could happen to other people too, where the OptionType already exists in the DB without it being on the product. I think this covers the existing behaviour fairly well, and ensures we don't get any duplicate records.
If there's a quicker way to link the record created by the initialize on the product from the original code to the DB let me know.