Skip to content

Add permalink history for taxon on friendly-id #6100

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

Merged

Conversation

JustShah
Copy link
Contributor

@JustShah JustShah commented Feb 5, 2025

[WIP] Open Questions

This PR aligns URL handling of taxons with products.

Open questions:

  • The slugs of taxons are currently called permalinks (which they aren't), should they be renamed to slug (in backend and DB) creating also a migration

Related Issues, PRs and Discussions

Summary

This PR introduces FriendlyId to the Taxon model to store the history of permalinks (slugs) and provides a more flexible approach to handling slugs for taxons.

Changes:

  • Taxon Model:

    • Added FriendlyId to the Taxon model, enabling it to generate and store slug history.
    • The before_save callback is used to set the permalink instead of before_create and before_update, allowing for better slug management.
    • Introduced a custom method for should_generate_new_friendly_id? to control when a new friendly ID should be generated, specifically when the permalink changes.
    • Added a custom normalize_friendly_id method to adjust how slugs are generated, especially for nested taxons, ensuring proper formatting of the slug.
  • Test:

    • Added a test to ensure that the history of permalinks is correctly stored when a taxon's permalink is updated. The test verifies that both the initial and updated slugs are retained in the history.

Motivation

This update enhances the management of taxon slugs by allowing historical tracking of slugs. This is particularly useful for SEO purposes and for maintaining accessibility of the resource if URLs change over time.

Note

The starter frontend PR is dependent for this change starter frontend PR

@JustShah JustShah requested a review from a team as a code owner February 5, 2025 14:08
@github-actions github-actions bot added the changelog:solidus_core Changes to the solidus_core gem label Feb 5, 2025
@JustShah JustShah closed this Feb 5, 2025
@JustShah JustShah reopened this Feb 5, 2025
Copy link

codecov bot commented Feb 5, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.68%. Comparing base (dfe87ae) to head (5618cc6).
Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6100   +/-   ##
=======================================
  Coverage   88.67%   88.68%           
=======================================
  Files         829      829           
  Lines       17995    18004    +9     
=======================================
+ Hits        15957    15966    +9     
  Misses       2038     2038           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@fthobe
Copy link
Contributor

fthobe commented Feb 5, 2025

@shahmayur001 i think there's a drop in cov across the project, we probably need to refine testing here.

@JustShah JustShah force-pushed the add-permalink-history-to-taxon branch 2 times, most recently from 69edb57 to 64d7be7 Compare February 5, 2025 15:25
@fthobe
Copy link
Contributor

fthobe commented Feb 9, 2025

@shahmayur001 as a consequence of this change, do we need to write logic to redirect {id} and {historyslug} as we did on products?

@fthobe fthobe force-pushed the add-permalink-history-to-taxon branch from 8fe563b to 70da3c7 Compare February 9, 2025 17:56
@JustShah JustShah force-pushed the add-permalink-history-to-taxon branch from 70da3c7 to 8722086 Compare February 10, 2025 09:30
@fthobe
Copy link
Contributor

fthobe commented Feb 10, 2025

@tvdeyen as bespoken history for Taxon slugs as well

@JustShah
Copy link
Contributor Author

@shahmayur001 as a consequence of this change, do we need to write logic to redirect {id} and {historyslug} as we did on products?

@fthobe Yes, we already included that changes in starter frontend PR.

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.

I explained it many times before and still I need to comment on every single PR of yours.

Please use your (often very good PR descriptions) as commit description(s). It is not even more work for you, because GitHub actually uses the git commit message to create the PR description in single commit PRs like this.

So, please always use the commit description for explaining why a change has been made. Even for multi commit PRs. The PR description is only helpful for users of the GH UI, while commit messages are used every where else (in your code editor, in third party git clients, the git log in your terminal, during rebase, etc.). It should always be the single source of truth and not the GitHub GUI). This has been discussed in the developer community all over the globe for decades now and should have been inherited by every single developer finally. I am a bit frustrated that I have to repeat this every, single time.

So, please - again - rewrite your commit message so that it includes the content of your - actually very good - PR description.

I hope that I do not have to write the same comment on your PRs over and over again.

Thanks.

@JustShah
Copy link
Contributor Author

I explained it many times before and still I need to comment on every single PR of yours.

Please use your (often very good PR descriptions) as commit description(s). It is not even more work for you, because GitHub actually uses the git commit message to create the PR description in single commit PRs like this.

So, please always use the commit description for explaining why a change has been made. Even for multi commit PRs. The PR description is only helpful for users of the GH UI, while commit messages are used every where else (in your code editor, in third party git clients, the git log in your terminal, during rebase, etc.). It should always be the single source of truth and not the GitHub GUI). This has been discussed in the developer community all over the globe for decades now and should have been inherited by every single developer finally. I am a bit frustrated that I have to repeat this every, single time.

So, please - again - rewrite your commit message so that it includes the content of your - actually very good - PR description.

I hope that I do not have to write the same comment on your PRs over and over again.

Thanks.

Thank you for the feedback. I was planning to update the commit message after addressing the comments but I completely agree with your point. Moving forward, I will ensure that commit includes the description. I appreciate your patience.

@tvdeyen
Copy link
Member

tvdeyen commented Feb 10, 2025

Thank you for the feedback. I was planning to update the commit message after addressing the comments but I completely agree with your point. Moving forward, I will ensure that commit includes the description. I appreciate your patience.

Thanks for your understanding. I actually is very helpful to have all the information beforehand in order to be able to review the PR in the first place.

- 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.
@JustShah JustShah force-pushed the add-permalink-history-to-taxon branch from 8722086 to 5618cc6 Compare February 10, 2025 10:36
@elia elia requested a review from tvdeyen February 10, 2025 12:16
@tvdeyen tvdeyen added this to the 4.5 milestone Feb 10, 2025
@tvdeyen tvdeyen merged commit efe8b97 into solidusio:main Feb 10, 2025
21 checks passed
@fthobe
Copy link
Contributor

fthobe commented Feb 10, 2025

@tvdeyen thank you

@fthobe
Copy link
Contributor

fthobe commented Feb 10, 2025

@tvdeyen there was a question on top of the PR, could you please answer to that so that we know what to do there. I understand that this is a breaking change, but I feel like it should be tackled sooner or later as current naming conventions between products and taxons regarding URLs are confusing.

@tvdeyen
Copy link
Member

tvdeyen commented Feb 10, 2025

We can do that, but is it worth the effort?

Because this is would we would have to do

  1. Introduce the new column
  2. Deprecate the old column
  3. Write data into both columns
  4. Stop writing the data into the old column
  5. Drop old column

All that over the period of at least one major version. I am not sure if this is really worth it. What is the reasoning behind this? Just because or is there a technical issue that this solves?

@fthobe
Copy link
Contributor

fthobe commented Feb 10, 2025

We can do that, but is it worth the effort?

Because this is would we would have to do

  1. Introduce the new column
  2. Deprecate the old column
  3. Write data into both columns
  4. Stop writing the data into the old column
  5. Drop old column

All that over the period of at least one major version. I am not sure if this is really worth it. What is the reasoning behind this? Just because or is there a technical issue that this solves?

In this case absolutely only bringing naming in line between resources. I wouldn't die on that hill. Taxon rendered is taxon rendered no matter the name. We could start by writing into both columns though and I mean that as suggestion not by any means as something that needs to be done now.

@tvdeyen
Copy link
Member

tvdeyen commented Feb 10, 2025 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:solidus_core Changes to the solidus_core gem
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants