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

Added GTIN and Condition to variant for structured data use #6097

Merged
merged 2 commits into from
Feb 19, 2025

Conversation

rahulsingh321
Copy link
Contributor

Introduction of Structured Data for Item Condition and Global Trade Item Number ("GTIN").

Contained Information

  • Brief Introduction into structured data
  • Detailed Description of added fields including 3rd party support
  • Screenshots of Admin Interface
  • Example of API request to post / get / update an item with the fields

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:

  • GTIN
  • ItemCondition

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

Search Engine Supported
Google YES
Bing YES
Yahoo YES
Yandex YES
Baidoo Partial / Closed Beta Program

Marketplaces

Search Engine Supported
Amazon YES
Facebook YES
Ebay YES
Etsy No
Vinted No

Search Ads

Search Engine Supported
Google Shopping YES
Bing YES (mandatory if given by manufacturer)
Yahoo YES
Yandex YES
Baidoo Unknown

Item Condition

Definition

The item condition has found wide acceptance over the past years to annotate if a product is

  1. New
  2. Used
  3. Refurbished
  4. Damaged

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

Search Engine Supported
Google YES
Bing YES
Yahoo YES
Yandex YES
Baidoo Partial / Closed Beta Program

Marketplaces

Search Engine Supported
Amazon YES (in addition Collectable)
Facebook YES (in addition various usage grades)
Ebay YES (with various additional information)
Etsy No
Vinted YES (Free field)

Search Ads

Search Engine Supported
Google Shopping YES
Bing YES (optional)
Yahoo YES
Yandex YES
Baidoo No

Concerns: #6060

Screenshots Admin Backend

Product

Screenshot from 2025-02-02 21-50-59

Variant

Screenshot from 2025-02-02 21-50-41

APIs

Get Product

Screenshot-from-2025-02-02-22-33-10-02-02-2025_10_33_PM

Update Product

Screenshot-from-2025-02-02-23-02-24-02-02-2025_11_02_PM

Possible Improvements

  • Validate GTIN against current schemes (EAN 9, UPC, ...)

@github-actions github-actions bot added changelog:solidus_api Changes to the solidus_api gem changelog:solidus_backend Changes to the solidus_backend gem changelog:solidus_core Changes to the solidus_core gem labels Feb 2, 2025
Copy link
Member

@kennyadsl kennyadsl left a 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.

@fthobe
Copy link
Contributor

fthobe commented Feb 3, 2025

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.

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?

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.

We can absolutely do that.

@kennyadsl
Copy link
Member

Could you elaborate your reasoning to ask?

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 condition field. With the way the feature is designed now, the best option is going with another field because it's very hard to extend.

@rahulsingh321
Copy link
Contributor Author

rahulsingh321 commented Feb 3, 2025

@kennyadsl

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.

Yes, I tried using the enum for this initially like this, but later moved to constant [ which I think give more flexibility to change ]
enum conditions: %i[damaged, new, refurbished, new ]

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 YAML.load_file, I can get that, that way, it can be easily used in different languages and extend a well. Let me know what do you think?

@fthobe
Copy link
Contributor

fthobe commented Feb 3, 2025

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 condition field. With the way the feature is designed now, the best option is going with another field because it's very hard to extend.

I See your point and would like to add some notes:

  • These values will be displayed in feeds and on frontend (via json snippets) as those are the two options given by 3rd parties short of api integration.
  • the values itself (conditions are I believe 8 years old by now) are very static, the implementation is not (the json might change down the road, the feeds are relatively stable in specs for many years as well)

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.

@fthobe
Copy link
Contributor

fthobe commented Feb 3, 2025

@rahulsingh321 after discussions with Kenny please enumerate the conditions.

@rahulsingh321 rahulsingh321 force-pushed the 6060-structured-data-products branch from 1ccd953 to 0365af4 Compare February 3, 2025 20:28
Copy link

codecov bot commented Feb 3, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.81%. Comparing base (08207da) to head (df607d8).

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.
📢 Have feedback on the report? Share it here.

@fthobe
Copy link
Contributor

fthobe commented Feb 3, 2025

@kennyadsl tests pass.

@mamhoff
Copy link
Contributor

mamhoff commented Feb 3, 2025

There are no tests for the new features. Please add them.

@fthobe
Copy link
Contributor

fthobe commented Feb 4, 2025

Thank you for your input. We will add them when we go out of draft.

@rahulsingh321 rahulsingh321 force-pushed the 6060-structured-data-products branch from d97fa5a to e04f42b Compare February 4, 2025 19:33
@fthobe
Copy link
Contributor

fthobe commented Feb 4, 2025

@mamhoff should better now

@rahulsingh321 rahulsingh321 force-pushed the 6060-structured-data-products branch 3 times, most recently from e04f42b to fe171d4 Compare February 6, 2025 12:06
@rahulsingh321 rahulsingh321 force-pushed the 6060-structured-data-products branch from fe171d4 to fdac605 Compare February 6, 2025 12:07
@rahulsingh321 rahulsingh321 force-pushed the 6060-structured-data-products branch from fdac605 to d44a9dd Compare February 6, 2025 19:35
@rahulsingh321 rahulsingh321 force-pushed the 6060-structured-data-products branch from d44a9dd to 53b229a Compare February 6, 2025 19:37
@rahulsingh321 rahulsingh321 marked this pull request as ready for review February 7, 2025 05:41
@rahulsingh321 rahulsingh321 requested a review from a team as a code owner February 7, 2025 05:41
@rahulsingh321 rahulsingh321 force-pushed the 6060-structured-data-products branch from 2ccc42a to b6cafe4 Compare February 11, 2025 14:56
@fthobe
Copy link
Contributor

fthobe commented Feb 12, 2025

@rahulsingh321 rebase and this should be fine!

@rahulsingh321 rahulsingh321 force-pushed the 6060-structured-data-products branch from b6cafe4 to cfcf1be Compare February 12, 2025 09:15
Copy link
Member

@tvdeyen tvdeyen left a 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.

@fthobe
Copy link
Contributor

fthobe commented Feb 12, 2025

@rahulsingh321

  • Add Master to the Labels on the product form till a universal decision has been made if to keep, adapt or drop master in favor of something else or anything at all.

  • Add translations if missing anywhere

Please squash and ammend the commit message as followed:
Adding Conditions and GTIN to products and variants

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
  • Added Testing of the related fields

@rahulsingh321 rahulsingh321 force-pushed the 6060-structured-data-products branch 2 times, most recently from 1bf5d31 to 0ea8609 Compare February 14, 2025 12:18
@tvdeyen
Copy link
Member

tvdeyen commented Feb 14, 2025

@rahulsingh321 do you mind to rebase this one more time? Thanks

@rahulsingh321 rahulsingh321 force-pushed the 6060-structured-data-products branch from b0012dd to 2937d9e Compare February 16, 2025 09:58
@fthobe
Copy link
Contributor

fthobe commented Feb 17, 2025

@kennyadsl punish me if I am lying, text is flaky PR is ok now.

@fthobe
Copy link
Contributor

fthobe commented Feb 17, 2025

@tvdeyen you ok with it now?

@tvdeyen tvdeyen force-pushed the 6060-structured-data-products branch from 2937d9e to 60da551 Compare February 18, 2025 14:45
@tvdeyen
Copy link
Member

tvdeyen commented Feb 18, 2025

Rebasing once more

Copy link
Member

@tvdeyen tvdeyen left a 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.

@tvdeyen
Copy link
Member

tvdeyen commented Feb 18, 2025

@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.

@rahulsingh321 rahulsingh321 force-pushed the 6060-structured-data-products branch from 60da551 to df607d8 Compare February 18, 2025 19:44
@tvdeyen tvdeyen added this to the 4.5 milestone Feb 19, 2025
rahulsingh321 and others added 2 commits February 19, 2025 10:47
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.
@tvdeyen tvdeyen force-pushed the 6060-structured-data-products branch from 4605ec1 to de38808 Compare February 19, 2025 09:47
@fthobe
Copy link
Contributor

fthobe commented Feb 19, 2025

thank you @tvdeyen

Copy link
Member

@kennyadsl kennyadsl left a 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!

@tvdeyen tvdeyen merged commit acd092f into solidusio:main Feb 19, 2025
20 of 21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:solidus_admin changelog:solidus_api Changes to the solidus_api gem changelog:solidus_backend Changes to the solidus_backend gem changelog:solidus_core Changes to the solidus_core gem changelog:solidus_sample Changes to the solidus_sample gem
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants