Skip to content

Conversation

Nevelito
Copy link
Contributor

@Nevelito Nevelito commented Feb 26, 2025

Description

Add bulk update to app

Changes:

  1. create new controller
  2. adjust the code in edit resource component to handle normal edit and update and bulk edit
  3. make changes in base controller to handle bulk update and fix codeclimat errors
  4. add new routes
  5. add new function to url_halpers.rb to handle bulk update
  6. adjust item_select_all_controller.js to handle adding resource ids to Bulk update button
  7. add locales
  8. add system specs

Fixes # (issue)
#3679

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works

Screenshots & recording

Screencast.from.12.03.2025.20.44.54.webm

Copy link

qlty-cloud-legacy bot commented Feb 26, 2025

Code Climate has analyzed commit d09bbb7 and detected 0 issues on this pull request.

View more on Code Climate.

@Nevelito Nevelito changed the title [WIP] feature: add bulk update feature: add bulk update Mar 12, 2025
Copy link
Contributor

This PR has been marked as stale because there was no activity for the past 15 days.

Copy link
Contributor

@Paul-Bob Paul-Bob left a 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 across edit and prefill_fields. I consolidated that logic into prefill_fields, which now returns a unified hash. Common values are preserved, mismatches become nil. 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 in self.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) {

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.

@Nevelito Nevelito requested a review from Paul-Bob June 16, 2025 15:16
@Nevelito
Copy link
Contributor Author

@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.

Copy link
Contributor

@Paul-Bob Paul-Bob left a 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.

@Nevelito Nevelito requested a review from Paul-Bob June 17, 2025 09:49
@Nevelito
Copy link
Contributor Author

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!

@Paul-Bob
Copy link
Contributor

Paul-Bob commented Jul 1, 2025

Hi @Paul-Bob I'm going on vacation for two months starting in two weeks.

Sounds great! Enjoy your time off!

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!

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.

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

Successfully merging this pull request may close these issues.

2 participants