-
-
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 structured data fields to Spree stores #6113
base: main
Are you sure you want to change the base?
Conversation
bb18ebd
to
c31ef6d
Compare
5221825
to
4d128b4
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6113 +/- ##
==========================================
+ Coverage 86.64% 88.86% +2.21%
==========================================
Files 514 838 +324
Lines 11894 18234 +6340
==========================================
+ Hits 10306 16203 +5897
- Misses 1588 2031 +443 ☔ View full report in Codecov by Sentry. |
747e3ab
to
5ca04d7
Compare
2b32fbd
to
3a6f46a
Compare
179b49e
to
fcce420
Compare
34599c8
to
a0b341d
Compare
c84b99e
to
bbff7bc
Compare
![]() |
@rahulsingh321 in future PRs I kindly ask you to follow @tvdeyen more on the commit structure. |
76b10b9
to
a7fa008
Compare
a7fa008
to
fc8e744
Compare
@tvdeyen @kennyadsl looks good to me |
This Commit provides the possibility to store legal information in the store resource. Given the wide array of different information required by country, the information stored here provides a baseline for most countries, but might need to be augmented in some jurisdictions. This commit is not meant as legal advisory. Inform yourself about eventual requirements in your country of operation. Some additional information that might be needed: - Legal Representative, - Share Capital of the company, - Contacts for privacy. Schema.org has a schema to markup organizations that has been widely adopted by various search engines: | | https://schema.org/Organization Support / Docs | |--------|-----------------------------------------------------------------------------------| | Bing | https://www.bing.com/webmasters/help/?topicid=cc507c09 | | Google | https://developers.google.com/search/docs/appearance/structured-data/organization | | Yandex | https://yandex.com/support/webmaster/schema-org/what-is-schema-org.html | This update allows to return all organization data on frontend via jsonb if implemented according to schema documentation on a per store basis and edit the data via API and Backend (Old / New). The implemented fields are: - legal_name, - contact_email, - contact_phone, - vat_id, - tax_id, - address1, - address2, - Zip code and city, - state_name, - country_id, - and state_id. Some additional notes regarding the size of the commit: While this commit seems massive in size, no functionality has been added apart from storing the values, no logical changes have been made, hence the commit while bigger in size is simple to analyse. Apart from testing routines, this commit does not contain functional code. Where possible the structure of current address forms has been adopted. Fixes solidusio#6173
fc8e744
to
a1b1502
Compare
@kennyadsl can we review this, it should be good to go. |
@fthobe please stop framing PRs and issues "your way" in the PR title. Thanks. |
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.
We should use the existing address form component, instead of introducing a second one. Also the PR adds a general new admin form for managing stores, which is very much appreciated, but this should at least be a dedicated commit the extra builds on top of and should no be combined into one commit, otherwise it is hard to see what the actual change (from your PR title) is.
@@ -0,0 +1,56 @@ | |||
import { Controller } from "@hotwired/stimulus" |
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.
We already have a address form component that handles states. Can we use that one instead of implementing a second one?
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.
Yes, we can look into reusing the logic to pick a state. Does it make sense if everything else should be detached from the address table?
<%= form_for @store, url: solidus_admin.store_path(@store), html: { id: form_id } do |f| %> | ||
<%= page_with_sidebar do %> | ||
<%= page_with_sidebar_main do %> | ||
<%= render component("ui/panel").new(title: t(".store_settings")) do %> |
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.
Why haven't you used the existing address form component?
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.
It didn't seem indicated to invoke the address list for performance reasons.
These values should be cached and provided separately as otherwise the address tabled needs to be used for values that in some scenarios could be embedded on every page depending on the implementation.
We can separate the new admin from the rest. |
<%= render component("ui/forms/field").text_field(f, :seo_title) %> | ||
<%= render component("ui/forms/field").text_field(f, :meta_keywords) %> | ||
<%= render component("ui/forms/field").text_area(f, :meta_description) %> | ||
<%= render component("ui/forms/field").text_field(f, :tax_id) %> |
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.
store_tax_id instead of tax_id
<%= render component("ui/forms/field").text_field(f, :meta_keywords) %> | ||
<%= render component("ui/forms/field").text_area(f, :meta_description) %> | ||
<%= render component("ui/forms/field").text_field(f, :tax_id) %> | ||
<%= render component("ui/forms/field").text_field(f, :vat_id) %> |
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.
store_vat_id instead of vat_id
@@ -354,18 +354,31 @@ en: | |||
quantity: Quantity | |||
variant: Variant | |||
spree/store: | |||
address: Address |
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.
store_...
<%= form_for @store, url: solidus_admin.store_path(@store), html: { id: form_id } do |f| %> | ||
<%= page_with_sidebar do %> | ||
<%= page_with_sidebar_main do %> | ||
<%= render component("ui/panel").new(title: t(".store_settings")) do %> |
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.
It didn't seem indicated to invoke the address list for performance reasons.
These values should be cached and provided separately as otherwise the address tabled needs to be used for values that in some scenarios could be embedded on every page depending on the implementation.
@@ -0,0 +1,56 @@ | |||
import { Controller } from "@hotwired/stimulus" |
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.
Yes, we can look into reusing the logic to pick a state. Does it make sense if everything else should be detached from the address table?
@tvdeyen replied to your comments |
The name is ok, the "[Tag]"-Part is unnecessary |
Summary
This Commit provides the possibility to store legal information in the
store resource. Given the wide array of different information required
by country, the information stored here provides a baseline for most
countries, but might need to be augmented in some jurisdictions. This
commit is not meant as legal advisory. Inform yourself about eventual
requirements in your country of operation.
Some additional information that might be needed:
Schema.org has a schema to markup organizations that has been widely
adopted by various search engines:
This update allows to return all organization data on frontend via jsonb
if implemented according to schema documentation on a per store basis
and edit the data via API and Backend (Old / New).
The implemented fields are:
Some additional notes regarding the size of the commit:
While this commit seems massive in size, no functionality has been added
apart from storing the values, no logical changes have been made, hence
the commit while bigger in size is simple to analyse. Apart from testing
routines, this commit does not contain functional code. Where possible
the structure of current address forms has been adopted.
Fixes #6173
Screenshot
Checklist
Check out our PR guidelines for more details.
The following are mandatory for all PRs:
The following are not always needed: