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 gem awesome_nested_set #11636

Closed
mkllnk opened this issue Oct 8, 2023 · 17 comments · Fixed by #12749
Closed

Remove gem awesome_nested_set #11636

mkllnk opened this issue Oct 8, 2023 · 17 comments · Fixed by #12749
Assignees
Labels
good first issue hackathon Issues for upcoming hackathons tech debt

Comments

@mkllnk
Copy link
Member

mkllnk commented Oct 8, 2023

What we should change and why (this is tech debt)

It's providing a tree structure for Spree::Taxon but we don't use the tree structure. I think that the UI is actually not saving it when you try. If we ever wanted a tree structure in the future, it would be better to use ancestry for it's simpler design and better update performance.

There are several related issues that may be resolved at the same time:

Context

Impact and timeline

No significant impact at the moment. But simplifying code always reduces maintenance and performance.

@github-project-automation github-project-automation bot moved this to To triage (By the maintainers) in Welcome New Developers! Oct 8, 2023
@github-project-automation github-project-automation bot moved this to All the things in OFN Delivery board Oct 8, 2023
@mkllnk mkllnk moved this from To triage (By the maintainers) to Backend Easy in Welcome New Developers! Oct 8, 2023
@sigmundpetersen sigmundpetersen added the hackathon Issues for upcoming hackathons label Oct 11, 2023
@HillaryOkello
Copy link
Contributor

Can I work on this?

@sigmundpetersen
Copy link
Contributor

Sure thanks @HillaryOkello , go ahead 🙂

@HillaryOkello
Copy link
Contributor

HillaryOkello commented Nov 15, 2023

I created a PR for this issue but I'm having an issue with the tests, I'm working on fixing them. I would appreciate some help #11808

@sigmundpetersen
Copy link
Contributor

Yes, thank you @HillaryOkello
The PR is waiting on code review, so you will get feedback from a dev as soon as they get to it 👍

@HillaryOkello
Copy link
Contributor

Thank you for the response @sigmundpetersen
I will keep trying to fix them as I wait for a code review 😄 👍

@HillaryOkello
Copy link
Contributor

Hello @sigmundpetersen,

Sorry, I've taken too long on this issue. I took a break and I'm back now.
However, I find this issue to be more complicated because many changes are required and removing some pieces of code is affecting some areas I don't understand quite well yet.

Is it okay if I pick another issue to work on?

@sigmundpetersen
Copy link
Contributor

Hey @HillaryOkello
Sure, no worries! Just pick another one 👍

@sigmundpetersen sigmundpetersen moved this from In progress to Backend Easy in Welcome New Developers! Jan 10, 2024
@sigmundpetersen
Copy link
Contributor

Thank you for your efforts on this one and thanks for the feedback!

@thebigw4lrus
Copy link

hey @sigmundpetersen I want to take over this one to warm up. WDYTH?

@sigmundpetersen
Copy link
Contributor

Sure thanks @thebigw4lrus , go ahead! 🎉 Thank you
Don't hesitate to ask if there's anything!

@sigmundpetersen sigmundpetersen moved this from Backend Easy to In progress in Welcome New Developers! Mar 12, 2024
@sigmundpetersen sigmundpetersen moved this from All the things to In Dev 💪 in OFN Delivery board Mar 12, 2024
@thebigw4lrus
Copy link

thebigw4lrus commented Mar 14, 2024

hey @sigmundpetersen , as I started digging into this issue, I got a few questions:

  1. I see there is a taxonomy model, related 1-n with taxons models. I see in the admin interface once I create a taxonomy, two entities will be created: Taxonomy and Taxons. If we try to remove the tree representation of taxonomies, it looks to me that taxon purpose was only to model this. Should we remove taxon model and stick only with taxonomy?.

  2. Looks like the Frontend needs to change as well: Now the Frontend tries to handle the trees unsuccessfully (it does not create them). is this part of the scope of the ticket?.

The way I see it there are two ways to move forward:

  1. Remove the Gem, Clean the database structure for taxon model, and clean every single call to "parent" or "children" and assume a flat model.

  2. The above (remove the gem), including also the FE changes (It would turn into a simple CRUD of taxonomy) and removing Taxon model. This way hypothetical Admin Simple Taxonomy CRUD would match the database structure.

2.1. The above (remove the gem), but keep Taxonomy-Taxon 1.N Relationship, meaning, we should adjust the UI in the scope of this ticket for a single taxonony to have several taxons.

I can do any way, but I am asking this to reassure on the ticket scope, as if we go for the path number 2, It would imply changes in several controllers outside of the taxon controller itself.

Thanks!

@sigmundpetersen
Copy link
Contributor

Thanks @thebigw4lrus .
Let's ask the @openfoodfoundation/core-devs if they have any feedback on this 👍

@mkllnk
Copy link
Member Author

mkllnk commented Mar 14, 2024

Very good analysis, thank you!

Should we remove taxon model and stick only with taxonomy?

It's the other way round. Taxons are referenced by products and they contain important data. The taxonomy is a group of taxons and not used anywhere. It can be removed, I believe.

All the taxon fields relating to the tree structure can also be removed. We may need to keep the position for ordering, I'm not sure.

Looks like the Frontend needs to change as well

Yes, this is part of the scope. It can be a complete rewrite and it can be super simple like a standard Rails view to edit the taxon model (index, create, update). Possibly with positioning, not sure.

Does this give you a clear path forward?

@thebigw4lrus
Copy link

thebigw4lrus commented Mar 18, 2024

It does @mkllnk , starting my engines then 🚀 .. Thanks tons!

@sigmundpetersen
Copy link
Contributor

Hi @thebigw4lrus hope everything is fine 🙂
Do you still want to work on this?

@RachL RachL moved this from In progress to Backend Easy in Welcome New Developers! Jun 7, 2024
@RachL RachL moved this from In Progress ⚙ to All the things 💤 in OFN Delivery board Jun 7, 2024
@wandji20
Copy link
Contributor

wandji20 commented Aug 1, 2024

I would love to work on this one.

@mkllnk mkllnk moved this from All the things 💤 to In Progress ⚙ in OFN Delivery board Aug 1, 2024
@mkllnk
Copy link
Member Author

mkllnk commented Aug 1, 2024

Great, I assigned you, @wandji20.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment