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

Remove :permalink attribute from Product #11137

Conversation

Matt-Yorkley
Copy link
Contributor

@Matt-Yorkley Matt-Yorkley commented Jun 28, 2023

What? Why?

Removes :permalink attribute from Product as discussed in #9069

It turns out it was being utilised in a couple of weird places, but not for anything useful; mostly in building URLs that get used for navigating to the admin edit products page.

What should we test?

Not sure what to test here? The URLs on admin edit product page will use the product ID instead of a permalink based on the name, eg /admin/products/18237/edit instead of /admin/products/bag-of-carrots/edit

Release notes

Changelog Category: Technical changes

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

@Matt-Yorkley Matt-Yorkley self-assigned this Jun 28, 2023
@Matt-Yorkley Matt-Yorkley force-pushed the remove-product-permalink branch 7 times, most recently from 4b090b3 to 0a1d7e0 Compare June 28, 2023 15:17
@Matt-Yorkley Matt-Yorkley marked this pull request as ready for review June 28, 2023 16:51
@drummer83
Copy link
Contributor

We have a funded feature for direct product links in the pipe. Just bringing it up here to avoid thinking: "If only we had some sort of permalink already." in a few weeks/months. 😉🤷‍♂️

@mkllnk
Copy link
Member

mkllnk commented Jun 30, 2023

We have a funded feature for direct product links in the pipe. Just bringing it up here to avoid thinking: "If only we had some sort of permalink already." in a few weeks/months. winkman_shrugging

Then we should implement that on the Spree::Variant model and allow linking straight to variants.

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.

Great! 🙏

Copy link
Collaborator

@rioug rioug left a comment

Choose a reason for hiding this comment

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

I love seeing all that code going away 🧹

@filipefurtad0 filipefurtad0 self-assigned this Jul 5, 2023
@filipefurtad0 filipefurtad0 added the pr-staged-fr staging.coopcircuits.fr label Jul 5, 2023
@filipefurtad0
Copy link
Contributor

filipefurtad0 commented Jul 5, 2023

Hey @Matt-Yorkley ,

Yup, that checks: The variant_id is used instead of the permalink, to navigate between pages 👌

Before the PR

image

After the PR

image

Quickly checked OCs, and checked out.
All good!

@filipefurtad0 filipefurtad0 merged commit 39b5970 into openfoodfoundation:master Jul 5, 2023
@filipefurtad0 filipefurtad0 removed the pr-staged-fr staging.coopcircuits.fr label Jul 5, 2023
@Matt-Yorkley Matt-Yorkley mentioned this pull request Jun 28, 2023
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants