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

Remove product SKU from product pages and report #12991

Merged
merged 1 commit into from
Jan 7, 2025
Merged

Remove product SKU from product pages and report #12991

merged 1 commit into from
Jan 7, 2025

Conversation

murjax
Copy link
Contributor

@murjax murjax commented Nov 20, 2024 •
edited
Loading

What? Why?

This removes the product SKU from the UI to remove ambiguity with the variant SKU.

Pages impacted:

  • Product List Page (/admin/products/)
  • Product Edit Page (/admin/products/:id/edit)
  • Orders and Distributors Report. (/admin/reports/orders_and_distributors)

What should we test?

  1. Sign in as an admin.
  2. Create a new product (Products -> New Product).
  3. Editing a product to add variants with SKUs. (Products -> Edit -> Variants -> New Variant)
  4. Viewing and editing a product from the list page, including the variant SKUs.

Release notes

Changelog Category (reviewers may add a label for the release notes):

  • User facing changes
  • API changes (V0, V1, DFC or Webhook)
  • Technical changes only
  • Feature toggled

The title of the pull request will be included in the release notes.

Dependencies

Documentation updates

mkllnk
mkllnk approved these changes Nov 21, 2024
View reviewed changes
Copy link
Member

@mkllnk mkllnk left a comment

Choose a reason for hiding this comment

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

Nice one. Thank you!

spec/system/admin/reports/orders_and_distributors_spec.rb
Comment on lines -37 to +38
"ABC", Spree::Product.first.name.to_s, "1g", "1", "none", "10.0", "none", "Check",
variant.sku, Spree::Product.first.name.to_s, "1g", "1", "none", "10.0", "none", "Check",
Copy link
Member

Choose a reason for hiding this comment

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

It's better to use the actual value in specs. Otherwise we may miss a bug, for example if the sku is nil and the report fails to print the sku then this test would still pass.

@mkllnk mkllnk added the user facing changes Thes pull requests affect the user experience label Nov 21, 2024
dacook
dacook approved these changes Nov 28, 2024
View reviewed changes
Copy link
Member

@dacook dacook left a comment

Choose a reason for hiding this comment

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

Looks good. I agree with Maikel's point but let's go.

@drummer83 drummer83 self-assigned this Jan 7, 2025
@drummer83 drummer83 added the pr-staged-au staging.openfoodnetwork.org.au label Jan 7, 2025
@drummer83
Copy link
Contributor

Hi @murjax,
Thank you so much for working on this. 🙏

I've done some testing and here are my results.

After staging

✔️ The field for SKU on master level is gone:
grafik

✔️ The SKU of the variant is used in the Orders and Distributors report:
grafik

✔️ The variant SKU can still be edited on /products and on the /products/-ID-/variants/-ID-/edit pages.
✔️ The master SKU can't be found in the UI.
✔️ Order confirmation emails and order cycle summary emails are using the variant SKU.
✔️ All other reports (where SKU is used) are using the variant SKU.

Results

I couldn't find any problems with this PR. It's working as expected. 🥳
I think we can go ahead and merge this one! 🚀
Thanks again! 🎉

@drummer83 drummer83 merged commit 8e0c039 into openfoodfoundation:master Jan 7, 2025
55 checks passed
@drummer83 drummer83 removed the pr-staged-au staging.openfoodnetwork.org.au label Jan 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dacook dacook dacook approved these changes

@mkllnk mkllnk mkllnk approved these changes

Assignees

@drummer83 drummer83

@murjax murjax

Labels
user facing changes Thes pull requests affect the user experience
Projects
OFN Delivery board
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Fully remove SKU on product level
4 participants
@murjax @drummer83 @mkllnk @dacook