-
-
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
Added GTIN and Condition to variant for structured data use #6097
Added GTIN and Condition to variant for structured data use #6097
Conversation
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 for the contribution. Out of curiosity, did you consider using an enum for the condition status? Not suggesting to use it (yet), just wondering if some reasoning around enum already happened, and in case, why it was discarded.
On a note for the future, I'd have appreciated two separate PR or at least commits. The reason is that we might need to revert one of the fields and it would take longer if the changes are mixed with some other feature. I think I'll live with the PR as is.
The value requested by third parties is literally the text String itself, which is well human readable. I felt enum would not have added value here, if there's something I am not seeing, we should make a change here. Could you elaborate your reasoning to ask?
We can absolutely do that. |
The reason is just not having the hardcoded strings in the database. I understand that you are looking at this in terms of 3rd party platform reading data, but there might be someone displaying those fields on the frontend, and they might have more states to display. They could go with a new field or extend the already present |
Yes, I tried using the enum for this initially like this, but later moved to constant [ which I think give more flexibility to change ] But using that got some error at variant level when using - variant.damaged? causing error, it been already defined by some other enum. Also, if you want, I can keep conditions in yaml and using |
I See your point and would like to add some notes:
If you feel like it adds value where platforms require additional values (eBay has like 8 states of used, Amazon 4). @kennyadsl if you happiness depends on it we can do it, if not I'd leave it like that. @rahulsingh321 wait for Kenny's reply and act accordingly. |
@rahulsingh321 after discussions with Kenny please enumerate the conditions. |
1ccd953
to
0365af4
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6097 +/- ##
=======================================
Coverage 88.81% 88.81%
=======================================
Files 835 835
Lines 18146 18150 +4
=======================================
+ Hits 16116 16120 +4
Misses 2030 2030 ☔ View full report in Codecov by Sentry. |
@kennyadsl tests pass. |
There are no tests for the new features. Please add them. |
Thank you for your input. We will add them when we go out of draft. |
d97fa5a
to
e04f42b
Compare
@mamhoff should better now |
e04f42b
to
fe171d4
Compare
fe171d4
to
fdac605
Compare
fdac605
to
d44a9dd
Compare
d44a9dd
to
53b229a
Compare
admin/app/components/solidus_admin/products/show/component.html.erb
Outdated
Show resolved
Hide resolved
2ccc42a
to
b6cafe4
Compare
@rahulsingh321 rebase and this should be fine! |
b6cafe4
to
cfcf1be
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 have two changes I would like to be addressed.
Please squash and ammend the commit message as followed: Summary This commit adds GTIN and Conditions to products to allow a Changes
|
1bf5d31
to
0ea8609
Compare
@rahulsingh321 do you mind to rebase this one more time? Thanks |
b0012dd
to
2937d9e
Compare
@kennyadsl punish me if I am lying, text is flaky PR is ok now. |
@tvdeyen you ok with it now? |
2937d9e
to
60da551
Compare
Rebasing once 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.
The enum definition is a bit confusing imo. See my comment below.
core/db/migrate/20250201172950_add_gtin_and_condition_to_spree_variant.rb
Show resolved
Hide resolved
@fthobe @rahulsingh321 as commented above we cannot use the Rails enum to store the values like that. We need to either have a getter def condition
read_attribute(:condition).capitalize
end or convert/translate the value where you actually generate your feed/API and not in the database model. So, please define the enum as enum :condition, { damaged: "damaged", new: "new", refurbished: "refurbished", used: "used" }, prefix: true That way the value in the database and the enum methods all use lowercased characters. |
60da551
to
df607d8
Compare
Summary This commit adds GTIN and Conditions to products to allow a more efficient integration of structured data and the related services such as Google Shopping or Facebook Marketplaces. Changes - Add Conditions and GTIN to products and variants - Added the fields to the Admin Interface - Add the values to API - Provides samples for GTIN (format EAN 8) and Condition (New) to all sample products. - Added Testing of the related fields - feat(i18n): Differentiate GTIN and Condition labels for product and variant forms.
Reflect the changes to the conditions values.
4605ec1
to
de38808
Compare
thank you @tvdeyen |
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 to everyone involved in this PR!
Introduction of Structured Data for Item Condition and Global Trade Item Number ("GTIN").
Contained Information
Introduction
This is the first in a series of PRs to implement full structured data support on Solidus.
Structured Data is the common standard to exchange product and content information via the frontend with search engines and marketing platforms and is defined by the schema.org organisation (through the founding companies Google, Microsoft, Yahoo and Yandex).
It has become the defacto standard to communicate with search engines and also many social networks (such as Facebook).
In this first PR following fields are tackled:
This scope of this PR is not to provide structured data on the frontend but merely to provide the information required to generate fully compliant structured data on frontend and feeds without 3rd party extensions or added fields.
GTIN (frequently also known as UPC / EAN in Europe)
Definition
A unique identifier for products usually given by either the manufacturer or the official distributor of a product. Frequently required to have stock listed with marketplaces. It's usually used to identify a product uniquely across multiple steps in the supply chain. While the GTIN is a unique identifier for a product entity it is in no mean a serial number (eg every iPhone of the same model and configuration sold in one country share the UPC, but not the serial number).
It's frequently used to match products in stock synchronizations or order APIs.
Support
Search Engines
Marketplaces
Search Ads
Item Condition
Definition
The item condition has found wide acceptance over the past years to annotate if a product is
While the origins where originally found in second hand marketplaces and the car industry, it became widely used with the emergence of the circular economy.
Support
Search Engines
Marketplaces
Search Ads
Concerns: #6060
Screenshots Admin Backend
Product
Variant
APIs
Get Product
Update Product
Possible Improvements