-
-
Notifications
You must be signed in to change notification settings - Fork 297
feature: add bulk update #3695
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
base: main
Are you sure you want to change the base?
feature: add bulk update #3695
Conversation
Code Climate has analyzed commit d09bbb7 and detected 0 issues on this pull request. View more on Code Climate. |
This PR has been marked as stale because there was no activity for the past 15 days. |
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.
This PR is taking longer than usual because it's a more substantial feature, and we need to be careful with the complexity it introduces. Typically, we build things iteratively, meaning the first iteration should be clean and simple enough to maintain and extend later. Thanks so much for your patience with all the back-and-forth on this one, @Nevelito .
I went ahead and pushed some tweaks around areas I consider important for the first version. You can see it in the commit history, but I'm summarizing the key changes here:
-
I noticed
@fields
was being used to store a hash of attributes and values. In Avo, we refer to variables as fields only when they are actual Avo fields. -
The method
prefill_fields
was receiving two arguments, but it was only ever called once, using instance variables. I updated it to take zero arguments and use the instance variables directly. -
The logic to construct the final hash with prefilled and
nil
values was split acrossedit
andprefill_fields
. I consolidated that logic intoprefill_fields
, which now returns a unified hash. Common values are preserved, mismatches becomenil
. For example, for two projects that only share the same progress value (7), the result would be:{ "id"=>nil, "name"=>nil, "status"=>nil, "stage"=>nil, "budget"=>nil, "country"=>nil, "users_required"=>nil, "started_at"=>nil, "meta"=>nil, "created_at"=>nil, "updated_at"=>nil, "description"=>nil, "progress"=>7 }
-
Since the contents weren't actually Avo fields, I renamed the method to
bulk_values
, and added a comment explaining what it does for clarity. -
I found that
set_fields
was unnecessary. It just grabbed the first record and its attributes. I replaced it with a direct call to the resource model class. I'm aware there was a conditional guard there, and I'll come back to that in this review with an improvement that makes the guard redundant. -
I switched the direct assignment of the record on the resource to use the hydration pattern. It's not essential, but aligns better with our not documented guidelines.
-
The
edit
response now uses the computed component approach like the base controller does. This gives developers flexibility to set custom bulk edit components, while defaulting to the resource's edit component. This needs to be documented inself.components
.
Since the reasoning behind these changes might not be fully clear from the commits alone, I felt it was worth writing it out here.
During this review process, I also noticed a few UI/UX aspects that need improvement:
- The bulk edit button should be part of the index controls, alongside the create and actions buttons, rather than being manually inserted into the filters area.
- It should be disabled until at least one record is selected (similar to how non-standalone actions work), and re-disabled when all records are deselected. This also ties into the earlier point about the conditional guard, we're now preventing code execution without a valid query, so the guard becomes unnecessary. In fact, if a blank query does slip through, we want it to raise an error because that should never happen.
On the topic of the query itself: I noticed we're passing it directly through params, which opens the door for malicious actors to tamper with the payload and possibly target records beyond their authorization scope. This could be mitigated using the built-in encryption service, but that's a larger topic. For now, let's focus on the two points I just mentioned.
Thanks again for your patience throughout this PR. It's a big one, and each review cycle takes time, but it's worth it to ensure we get it right.
this.updateActionLinks(param, 'a[href*="/admin/bulk_update/edit"]') | ||
} | ||
|
||
updateActionLinks(param, selector) { |
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.
Function updateActionLinks
has 30 lines of code (exceeds 25 allowed). Consider refactoring.
@Paul-Bob have a look at new changes, I think it starting to look as it should. There is some issue with system spec in old rails version but I will try to figure it out tomorrow. |
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.
The bulk_update_no_records
and bulk_update
translations don't seem to be used anymore.
Other than that, I think we're really close to completing this PR.
Hi @Paul-Bob I'm going on vacation for two months starting in two weeks. It would be great to wrap up this feature before then. Could you please take another look and let me know if there's anything I can do on my side to move it forward? Thanks in advance! |
Sounds great! Enjoy your time off!
All good on your end, thanks so much for the effort you put into this! I'll take it from here and wrap up the PR. There are still a few loose ends and the last time I spoke with Adrian, we discussed possibly moving this PR to a private repo. Really appreciate all the work you put in, this one was a bit of a beast! Let me know if there's another issue you'd like to pick up next. |
Description
Add bulk update to app
Changes:
Fixes # (issue)
#3679
Checklist:
Screenshots & recording
Screencast.from.12.03.2025.20.44.54.webm