For super admin, add pagination on /admin/enterprises
#10322
Conversation
jibees
force-pushed
the
8027-performance-improvement-on-adminenterprises
branch
4 times, most recently
from
56230be to
5b83429
Compare
|
|
||
| # These need to run before #load_resource so that @object is initialised with sanitised values | ||
| prepend_before_action :override_owner, only: :create | ||
| prepend_before_action :override_sells, only: :create | ||
|
|
||
| before_action :load_enterprise_set, only: :index | ||
| before_action :load_enterprise_set_on_index, only: :index |
There was a problem hiding this comment.
When something is happening only in one action, I often prefer to call it within that action. It keeps related code together and you can see quicker what's happening.
Enterprises are stored in `@enterprise_set` variables, and we iterate over to show the list of enterprises to super admin. Previously, we used to use `Sets::EnterpriseSet.new(collection)` instead of creating set based on `@collection`: this leads to call the `collection` method twice, which was probably very time consuming. This commit fix also that. + use paginated enterprises loading on bulk update but without testing if the current user is an admin
so let's call it inside the action.
jibees
force-pushed
the
8027-performance-improvement-on-adminenterprises
branch
from
84e2fbc to
9b2ed88
Compare
| @pagy, @paginated_collection = pagy(@collection) | ||
| @enterprise_set = Sets::EnterpriseSet.new(@paginated_collection, params) |
There was a problem hiding this comment.
I guess that @paginated_collection is not used anywhere else in the code. So by making it a local variable, it's clear that you can change the name or do whatever you like without affected other code.
On the other hand, this variable may be useful at some point. And if we publicise @pagy then why not the collection? So I don't mind this version either. It looks more consistent.
|
Hi @jibees, Before stagingAs admin user: As super admin user (loading takes like forever): After stagingAs admin user: As super admin user: Results
Note
Thanks! I think this is ready to merge! 🥳 |
What? Why?
Introducing pagination thanks to
pagyfor super admin on this page.Also, previously, we used to use
@enterprise_set = Sets::EnterpriseSet.new(collection)instead of creating set based on@collection: this leads to call thecollectionmethod twice, which was probably very time consuming. This commit fix also that.What should we test?
/admin/enterprises. In term of performance, actually, I don't know if this could be mesurable in staging. Maybe we could compare the request time of/admin/enterprisesURL via chrome|firefox inspector.Release notes
Changelog Category: User facing changes
The title of the pull request will be included in the release notes.