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

Improve admin/enterprise_roles page performance #11983

Merged
merged 4 commits into from
Jan 10, 2024
Merged

Improve admin/enterprise_roles page performance #11983

merged 4 commits into from
Jan 10, 2024

Conversation

bmd08a1
Copy link
Contributor

@bmd08a1 bmd08a1 commented Dec 26, 2023

What? Why?

What should we test?

  • Log in as super admin.
  • Visit /admin/enterprise_roles.

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

Improve admin/enterprise_roles page performance

@bmd08a1
Copy link
Contributor Author

bmd08a1 commented Dec 27, 2023

I think the proper fix is to add pagination on the @enterprise_roles and add 2 APIs to implement search for @users and @enterprises. For that, I would need someone to help with the FE changes because I have no knowledge in working with AngularJs

@mkllnk
Copy link
Member

mkllnk commented Jan 3, 2024

I would need someone to help with the FE changes because I have no knowledge in working with AngularJs

We are actually in the process of replacing AngularJS with StimulusReflex. Would you enjoy rewriting that page to remove AngularJS?

mkllnk
mkllnk approved these changes Jan 3, 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.

Great work!

Just a bit of cleanup needed.

app/views/admin/enterprise_roles/_data.html.haml
Comment on lines -1 to -2
= admin_inject_enterprise_roles(@enterprise_roles)
= admin_inject_users(@users)
Copy link
Member

Choose a reason for hiding this comment

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

It looks like these two helpers are now unused and can be deleted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated, thanks

@bmd08a1
Copy link
Contributor Author

bmd08a1 commented Jan 3, 2024

I would need someone to help with the FE changes because I have no knowledge in working with AngularJs

We are actually in the process of replacing AngularJS with StimulusReflex. Would you enjoy rewriting that page to remove AngularJS?

Ah sorry, I would love focus only with BE, I can help if you need to update APIs for those

mkllnk
mkllnk approved these changes Jan 3, 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.

No worries. Thanks for your contributions.

rioug
rioug requested changes Jan 3, 2024
View reviewed changes
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.

Great work ! I spotted a typo, could you fix that ?

app/queries/admin/enterprise_roles_query.rb Outdated Show resolved Hide resolved
@drummer83 drummer83 self-assigned this Jan 10, 2024
@drummer83 drummer83 added pr-staged-uk staging.openfoodnetwork.org.uk pr-staged-fr staging.coopcircuits.fr labels Jan 10, 2024
@drummer83
Copy link
Contributor

Hi @bmd08a1,
Wow, this is impressive! From timeout error to loading times of ~8 seconds! Well done!

I have tested this on staging UK and FR, both with similar results. The page is loading and it's really fast.

That's a great improvement! Merging and thanks again!

@drummer83 drummer83 merged commit 1c01af5 into openfoodfoundation:master Jan 10, 2024
54 checks passed
@drummer83 drummer83 removed pr-staged-uk staging.openfoodnetwork.org.uk pr-staged-fr staging.coopcircuits.fr labels Jan 10, 2024
@filipefurtad0 filipefurtad0 added the technical changes only These pull requests do not contain user facing changes and are grouped in release notes label Jan 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mkllnk mkllnk mkllnk approved these changes

@rioug rioug rioug approved these changes

Assignees

@drummer83 drummer83

@bmd08a1 bmd08a1

Labels
technical changes only These pull requests do not contain user facing changes and are grouped in release notes
Projects
OFN Delivery board
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Staging servers] Snail (error 504, timeout) when accessing the page /admin/enterprise_roles
5 participants
@bmd08a1 @mkllnk @drummer83 @rioug @filipefurtad0