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.

Sign up for GitHub

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

Jump to bottom

Structured data for store/organisation #6

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from
Draft

Structured data for store/organisation #6

wants to merge 8 commits into from

Conversation

rahulsingh321
Copy link

@rahulsingh321 rahulsingh321 commented Feb 3, 2025 •
edited
Loading

Store form UI:
Screenshot from 2025-02-03 23-31-47

jarednorman and others added 2 commits January 31, 2025 08:15
@jarednorman @adammathys
@AlistairNorman @forkata @harmonymjb @sofiabesenski4 @benjaminwil
Refactor line item total calculations
c261747 
While working on the in-memory updater in solidusio#5872, we found the need to
change how item totals were being calculated, so that we could mark
adjustments for destruction without actually destroying them, while
still keeping tax adjustments intact. This change is completely
backwards-compatible with the current OrderUpdater, so to reduce the
scope of our PR, we wanted to make this change separately.

Since the OrderUpdater is already very large, this helps reduce its
responsibilities and makes it easier to test this behaviour. We don't
see it as necessary to make this a configurable class, but this change
leaves that option open in the future.

Co-authored-by: Adam Mueller <[email protected]>
Co-authored-by: Alistair Norman <[email protected]>
Co-authored-by: Chris Todorov <[email protected]>
Co-authored-by: Harmony Bouvier <[email protected]>
Co-authored-by: Sofia Besenski <[email protected]>
Co-authored-by: benjamin wil <[email protected]>
@adammathys
Make ItemTotal class configurable
7a3bfc2 
I don't know if there's all that much customizing people will perform on
this class, but it's easy enough to make it configurable just in case.
@rahulsingh321 rahulsingh321 changed the title Structured data for store / organisation Structured data for store/organisation Feb 3, 2025
@fthobe
Copy link

fthobe commented Feb 3, 2025

@rahulsingh321 translations for labels are entirely missing.
Also here labels are acronyms:
VAT all capitalized as acronym for Value added tax therefor VAT ID
Tax ID not, because tax is not an acronym

@rahulsingh321
Copy link
Author

Todos:
1: Address optional in store.
2: Need to add the translation
3: Need to add the test cases.

@fthobe
Copy link

fthobe commented Feb 5, 2025

@kennyadsl we are unsure if using addresses or just make columns here.

Specs by google are following:

The address (physical or mailing) of your organization, if applicable. Include all properties that apply to your country. The more properties you provide, the higher quality the result is for users. You can provide multiple addresses if you have a location in multiple cities, states, or countries.

<html>
  <head>
    <title>About Us</title>
    <script type="application/ld+json">
    {
      "@context": "https://schema.org",
      "@type": "Organization",
      "url": "https://www.example.com",
      "sameAs": ["https://example.net/profile/example1234", "https://example.org/example1234"],
      "logo": "https://www.example.com/images/logo.png",
      "name": "Example Corporation",
      "description": "The example corporation is well-known for producing high-quality widgets",
      "email": "[email protected]",
      "telephone": "+47-99-999-9999",
      "address": {
        "@type": "PostalAddress",
        "streetAddress": "Rue Improbable 99",
        "addressLocality": "Paris",
        "addressCountry": "FR",
        "addressRegion": "Ile-de-France",
        "postalCode": "75001"
      },
      "vatID": "FR12345678901",
      "iso6523Code": "0199:724500PMK2A2M1SQQ228"
    }
    </script>
  </head>
  <body>
  </body>
</html>

@fthobe
Copy link

fthobe commented Feb 5, 2025 •
edited
Loading

@rahulsingh321 shoot at solidusio/solidus (main) as draft and check if tests fail. see below comment

@fthobe
Copy link

fthobe commented Feb 5, 2025

@rahulsingh321 wait for PR description by me before making pr

@fthobe
Copy link

fthobe commented Feb 6, 2025

@rahulsingh321 please update the screenshot

forkata and others added 3 commits February 7, 2025 15:12
@forkata @benjaminwil
Fix field container identifier on admin stock location
72af4a8 
The `address1` container incorrectly had the same identifier as the
"Check stock on transfer" input checkbox. This fixes the typo, so this
container can be referenced in Deface overrides, etc.

Co-authored-by: Benjamin Willems <[email protected]>
@shahmayur001
Add permalink history for taxon on friendly-id
5618cc6 
- Introduced FriendlyId to the Taxon model to enable slug history tracking.
- Replaced before_create and before_update callbacks with a before_save callback for better slug management.
- Implemented a custom should_generate_new_friendly_id? method to control when a new slug is generated, specifically when the permalink changes.
- Added a custom normalize_friendly_id method to ensure proper slug formatting for nested taxons.
- Included a test to verify that the history of permalinks is correctly stored when a taxon's permalink is updated, ensuring both initial and updated slugs are retained.

This change enhances the flexibility and reliability of handling slugs for taxons.
@tvdeyen
Merge pull request solidusio#6100 from gms-electronics/add-permalink-…
efe8b97 
…history-to-taxon

Add permalink history for taxon on friendly-id
@rahulsingh321 rahulsingh321 force-pushed the 5905-add-store-fields branch 2 times, most recently from bb18ebd to c31ef6d Compare February 10, 2025 19:37
@tvdeyen
Merge pull request solidusio#6107 from SuperGoodSoft/fix-typo-in-stoc…
ec1c421 
…k-location-form-partial

Fix field container identifier on admin stock location
fthobe
fthobe requested changes Feb 11, 2025
View reviewed changes
Copy link

@fthobe fthobe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally it looks good, some minor things:

  • try to align the section headers with what they do in the new admin
  • try to be more consistent with naming throughout a PR

Rest looks good, form is well structured, tests are there. I don't see why this wouldn't be ok. @shahmayur001 needs to remove the wrongly placed commits from here.

core/db/migrate/20250202173007_add_store_attributes_to_spree_stores.rb Outdated Show resolved Hide resolved
backend/app/views/spree/admin/stores/_form.html.erb Outdated Show resolved Hide resolved
backend/app/views/spree/admin/stores/_form.html.erb Outdated Show resolved Hide resolved
backend/app/views/spree/admin/stores/_form.html.erb Outdated Show resolved Hide resolved
api/spec/requests/spree/api/stores_spec.rb Outdated Show resolved Hide resolved
api/spec/requests/spree/api/stores_spec.rb Outdated Show resolved Hide resolved
api/lib/spree/api_configuration.rb Outdated Show resolved Hide resolved
backend/app/views/spree/admin/stores/_address_form.html.erb Outdated Show resolved Hide resolved
core/app/models/spree/taxon.rb Outdated Show resolved Hide resolved
core/lib/spree/permitted_attributes.rb Outdated Show resolved Hide resolved
@rahulsingh321 rahulsingh321 force-pushed the 5905-add-store-fields branch 2 times, most recently from 2567caf to 7cc9c0a Compare February 11, 2025 11:27
@jarednorman
Merge pull request solidusio#6080 from SuperGoodSoft/refactor-line-it…
95a9ea8 
…em-total-updates

Refactor Line Item Total Calculations
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shahmayur001 shahmayur001 shahmayur001 left review comments

@fthobe fthobe fthobe requested changes

Assignees
No one assigned
Labels
changelog:solidus_api changelog:solidus_backend changelog:solidus_core changelog:solidus_legacy_promotions
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@rahulsingh321 @fthobe @shahmayur001 @jarednorman @adammathys @forkata @tvdeyen