-
-
Notifications
You must be signed in to change notification settings - Fork 730
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 awesome nested set gem and dependencies [OFN-11636] #12749
Conversation
3c31056
to
10d436a
Compare
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.
Excellent work! Thank you.
It would have been easier to review if you had created several smaller commits to guide us through the changes step by step.
I'm adding one commit to fix up the migration but otherwise it's all good. There's one leftover jquery plugin now that we can remove as well. Would you like to tackle that?
remove_reference :spree_taxons, :parent, index: true, foriegn_key: true | ||
remove_reference :spree_taxons, :taxonomy, index: true, foriegn_key: true |
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 looks like a typo here:
remove_reference :spree_taxons, :parent, index: true, foriegn_key: true | |
remove_reference :spree_taxons, :taxonomy, index: true, foriegn_key: true | |
remove_reference :spree_taxons, :parent, index: true, foreign_key: true | |
remove_reference :spree_taxons, :taxonomy, index: true, foreign_key: true |
So I assume that you didn't test rolling back the migration and check that it reverts back to how it was.
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.
I'll add a commit to make the migration reversible.
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.
What about the Javascript part?
vendor/assets/javascripts/jquery.jstree/jquery.jstree.js
Oh, and it looks like you need to rebase this pull request on the latest master to resolve conflicts. |
While testing I noticed that the permalink is not visible. Setting a new value doesn't work either. I don't actually think that it's used anywhere. Can you remove at least the input field from the form? |
a3527e2
to
cf952c3
Compare
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.
Awesome, thanks ! 🙏
I'll second Maikel here, it would have been easier to follow if it was split in multiple small commit.
I think we'll need to flag this to the instance manager when it get released, they might want to review their taxonomy afterward.
Hi @wandji20, Configuration tabs in the instance settingsTaxonomies vs. Product Categories pageTaxonomy edit vs. Product Category edit pageHeader taxonomies and sub taxons listed as Product CategoriesAdditional test cases
Open (nice to have in a follow-up)
Task❗ Inform instance managers about this change. ConclusionEverything I tested is working well. Great job! This PR is ready to go. Merging! 🚀 |
Created #12773 for follow up issues. |
What? Why?
jquery.jstree
from the taxonomy page #9677Awesomenested set and related dependencies where removed
parent_id
,lft
, andrgt
where removed from Taxon modelWhat should we test?
Video
Screencast.from.07-08-2024.15.48.29.webm
http://localhost:3000/admin/general_settings/edit
Release notes
Removes unused tree structure on product categories. Your taxons and taxonomies will now be listed in one flat alphabetical list.
Changelog Category (reviewers may add a label for the release notes):
Dependencies
N/A
Documentation updates
N/A