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.
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
feat: add new site preference for indexing out-of-stock products #218
base: develop
Are you sure you want to change the base?
feat: add new site preference for indexing out-of-stock products #218
Changes from all commits
1b3ddc4
fd6f0aa
a003010
edcd392
deeddea
3ce475d
225e26d
2ef8208
00b71c7
135082f
48cd8ab
ace7106
09aff99
50341fd
2f3185e
ad7ea14
437ccb3
c411c27
648ad10
dfe7048
5336413
064e74a
b4cab2b
663304d
07e9442
d05b4fa
921e61c
626cb02
13c9a33
d702d6f
4c141d9
8ce35f7
e347eaf
2a614ec
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Actually please put this checkbox above all the frontend related settings (below InStock threshold is the best place)
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.
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.
Those are already loaded once in the constants at the top of the file:
ALGOLIA_IN_STOCK_THRESHOLD
andINDEX_OUT_OF_STOCK
, you can use themThere 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.
I put this comment before seeing the explanation in the tests. I think it worth refactoring and passing them as a parameter than fetching the preference from this file in the end.
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.
Now that you have already checked if the product is in stock, you can optimize the build of the record by passing a
baseModel
with thein_stock
value: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.
Now you have removed the
else
condition entirely, but it's needed:If the master product becomes out of stock, we want to delete it from the Algolia index.
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.
The condition is not what is documented:
You will end up here just if
inStock
isfalse
So if a product is not in stock but
INDEX_OUT_OF_STOCK
is true, it will be deleted!It's supposed to be covered by your scenario 29 which you indicated as passed:
Did you actually run the scenario?
I tested myself and the variant was removed from the index..
You also have a unit test for it but I see it's broken.
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.
Ok my bad the condition is actually a
||
and not a&&
, I probably made a mistake while testing, this works ok.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.
This preference has not been added to the Algolia group so it's not visible in the Custom Preferences of the site.
(side question: Do you know where I can see its value in that case?)
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.
answer for the side question: No, it is not possible via standard UI, only possible via our custom UI