-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Add primary Taxon to products (#6047) #6109
Add primary Taxon to products (#6047) #6109
Conversation
37502e3
to
0e48541
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6109 +/- ##
==========================================
- Coverage 88.68% 86.63% -2.06%
==========================================
Files 830 512 -318
Lines 18013 11819 -6194
==========================================
- Hits 15975 10239 -5736
+ Misses 2038 1580 -458 ☔ View full report in Codecov by Sentry. |
4290f45
to
1968ce9
Compare
1968ce9
to
fc7a95d
Compare
@shahmayur001 if you rebase I add the open api description |
fc7a95d
to
26d3df0
Compare
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.
Thanks. I left a comment on the placeholder. Also, do we need to add the same field in the new admin products/show?
I would yes |
It would be great to know if we still care about the old admin? Are new features still supported on it? |
@fthobe right now there is no edit page for products in the new admin, so we need to put them in the old one. |
Why? Seriously I am not arguing here, if the product editing page is ready let's use that one. Please pick one of these
|
The new admin is still in beta (and will probably be for a long time). We still need to support the So
and let's please use "current admin" as name for the current admin. It will be old (and obsolete) the moment we consider the new admin stable and ship it as default. |
Ok that's a clear statement. @shahmayur001 |
b7320de
to
fb02b5e
Compare
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.
Approved, but I just noticed we didn't add the primary taxon to sample products. Ok for me to add it in another PR if needed. Up to you!
I would actually make some adjustments to starter regarding brands and make one run on sample data as a total as we need it to test structured data. Is that acceptable for everybody? |
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.
Great work @shahmayur001
I have to change requests, then this can be merged.
core/db/migrate/20250207104016_add_primary_taxon_to_products.rb
Outdated
Show resolved
Hide resolved
f7f91c0
to
a456863
Compare
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.
Thanks. I just saw that the taxonAutocomplete jQuery function has been renamed for no obvious reason. Please revert this change or provide good reasons for the change and add backwards compatibility.
Also please create a dedicated commit that introduces the changes for the options that now can be passed in. This is a welcome change, but it needs a separate commit at least.
backend/app/assets/javascripts/spree/backend/taxon_autocomplete.js
Outdated
Show resolved
Hide resolved
a456863
to
3f204c4
Compare
Created a new association between Product and Primary Taxon using the existing Taxon model.
Allows access to the newly added attribute through existing product APIs.
Updated the product form for primary taxon and modified the related jQuery to display and select a specific taxon.
3f204c4
to
f35f325
Compare
@tvdeyen sorry for the mixup with the jscript, should be fine now. Test seems flaky, should I open a ticket to add a retry to
|
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.
Thanks.
Reran flaky spec. But this is unrelated |
Summary
Concerns #6047
Concerns Starter_Frontend #390 #450
Caution
To implement this feature on starter_frontend also #418 needs to be merged.
This PR fixes outcome variability on the breadcrumbs rendered on the frontpage and is part of a couple of PRs addressing the lack of some SEO features that became prevalent.
Issue: Breadcrumbs are not rendered reliably given that any of multiple taxons can be used
Breadcrumbs are used to tell search engines more about the overal page structure of a website. Currently the starter frontend pulls the first taxon identified on the website and renders that including all ancestors as breadcrumbs terminating the trail with the current page.
While this renders valid breadcrumbs (opposite to some other pages) it does neccessarily reinforce the desired keyword as the taxon rendered as breadcrumb does not always reflect the keyword desired.
To solve that issue @shahmayur001 implemented an additional instrument to select the desired breadcrumb taxon in the admin interface.
Fixes #6047
Checklist
Check out our PR guidelines for more details.
The following are mandatory for all PRs:
The following are not always needed: