Fix updating products failure when too many parameters are sent by chahmedejaz · Pull Request #14293 · openfoodfoundation/openfoodnetwork

chahmedejaz · 2026-05-17T20:21:28Z

What? Why?

Closes Updating products fails when too many parameters are sent #13713

This PR fixes bulk product updates failing with:

Invalid request parameters: total number of query parameters exceeds limit (4096)

The root cause was that the bulk update form submitted all product fields on the page, including unchanged records and nested variant attributes. When many products were displayed, the generated payload exceeded Rails/Rack's parameter limit and resulted in a ActionController::BadRequest error.

Solution

This change reduces the size of submitted bulk update payloads by submitting only changed fields and the minimum identity fields required for updates:

Disable unchanged form fields before submission in the bulk update form.
Preserve required product and variant identity fields (id) so updates can still be applied correctly.
Ensure related hidden/derived fields (for example unit values) remain included when associated fields change.
Normalize submitted bulk parameters server-side to support sparse submissions.
Infer product_id from submitted variant IDs when a product ID is omitted (for variant-only changes).

Additionally, this PR fixes an edge case in variant unit scale updates where changing the scale (e.g. g → kg) without modifying the visible quantity could lead to incorrect persisted values. The update flow now normalizes unit_value to preserve the visible quantity while still respecting explicitly submitted values.

Tests were added to cover:

Submitting only changed fields in bulk updates.
Variant-only updates with inferred product IDs.
Variant unit scale conversions and validation edge cases.

What should we test?

Visit the bulk product edit page (/admin/products).
Load a large product list that previously produced the parameter limit error.
Update a single product field and save.
Confirm the update succeeds without ActionController::BadRequest.
Update only a variant field (without changing product-level fields) and confirm changes persist.
Change a variant unit scale (for example g → kg) without modifying the quantity and confirm the visible quantity remains consistent after save.
Change both variant scale and quantity and confirm the explicitly entered quantity is respected.
Verify unchanged products/variants are not unintentionally modified.
Verify changing multiple products/variants on a single save and they all should be persisted after the save.
Also verify creating new variants

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

…s and enhance form submission handling for changed fields

…hanges

rioug

Looks good, thanks.
I am not super thrilled about the added complexity. I wonder, given we already have a frontend check for changed item, if we could keep a list of updated products/variants when they are changed, and use that list to create the data to send.
But that probably comes with another set of complexity, so who knows 🤷

Makes me wonder if we should have gone with one request per changed product/variants instead of using product sets...

rioug · 2026-05-22T02:36:59Z

+      elements.forEach((element) => {
+        if (element.type !== "submit") {
+          element.disabled = true;
+        }
+      });


~~Shouldn't we be doing this after we checked if any elements have been changed ? No point disabling element if we are not going to submit anything~~
Sorry I misunderstood what we are doing here, I see we are disabling element so that they don't get submitted. I am assuming element will get re enabled after the form reload.

Yes, Gaetan. This will only get triggered upon form submission and after that we are reloading the entire table that would reset the all elements state to the original one.

rioug · 2026-05-22T02:54:17Z

+      variant_ids = Array(variants_attributes).filter_map { |variant| variant[:id].presence }
+      return if variant_ids.empty?
+
+      Spree::Variant.where(id: variant_ids).pick(:product_id)


Is this to cover an edge case ? or is it likely we'll get lots of product is missing ? I fear this could generate a lots of queries and possibly result in performance issue.

In our case it shouldn't happen because I've made sure that we always include product and variant id in the payload, so the payload ids will be used rather than db query. However, just in case if the payload doesn't include product ids then we could use a db fallback.
Let me just double check this as well.

I double checked this, and yes, we are making sure that we send the product and variant id from the frontend in the request.

Thanks for checking !

If we are making sure that we always send the product ID with the Javascript, then maybe we don't need infer_product_id_from_variants anymore. Can it be removed?

dacook

Thanks Ahmed, I tried it out and it works beautifully. I updated 27 products and the payload was 1.6kb 👍

I did notice that the disabled fields have a grey border around them temporarily. I think the experience would be smoother if these didn't appear, can you hide them?

Also for the second part, I have an alternative idea, can you please check it out and if it looks good, update the code and specs?

dacook · 2026-05-26T00:55:59Z

+      variant_ids = Array(variants_attributes).filter_map { |variant| variant[:id].presence }
+      return if variant_ids.empty?
+
+      Spree::Variant.where(id: variant_ids).pick(:product_id)


If we are making sure that we always send the product ID with the Javascript, then maybe we don't need infer_product_id_from_variants anymore. Can it be removed?

dacook · 2026-05-26T01:27:01Z

+    relatedContainer
+      .querySelectorAll('input[type="hidden"], input[type="checkbox"], select, textarea')
+      .forEach((relatedElement) => this.#enableElement(relatedElement));
+  }


I'm guessing this was required to ensure unit value updates work.
It's quite specific and I worry that it might break in the future. Do you know if this specific case is covered by a system spec?

It makes me wonder if it would have been ok to take a simpler approach and enable the entire variant row if any fields are changed. But this implementation is much more efficient in the case where you update one field on lots of products at once.

Which elements exactly is this for? if there's a Stimulus controller for these elements, maybe it could be marking them as "changed", then you wouldn't need this extra logic here.

dacook · 2026-05-26T01:33:37Z

+          variant_unit: variant.variant_unit,
+          unit_scale: variant.variant_unit_scale,
+          unit_value: variant.unit_value,
+        }


I think Rails does this for you: https://api.rubyonrails.org/classes/ActiveModel/Dirty.html
For example variant.variant_unit_was

dacook · 2026-05-26T02:12:12Z

      variant&.errors.blank?
    end

+    def normalize_unit_value_for_scale_change(


I actually think it might be better to handle this on the browser side. It seems this code is intended to copy the behaviour on the browser, which automatically adjusts the unit value and displays it to the user.

The problem is that the hidden field unit_value changes, but doesn't get marked as changed because it's a hidden field.
We can't actually track changes on a hidden field but could work around by changing it to a text field with display:none
(see

openfoodnetwork/app/webpacker/controllers/bulk_form_controller.js

Lines 176 to 178 in 63255e6

// This doesn't work with hidden field

// Workaround: use a text field with "display:none;"

return element.defaultValue !== undefined && element.value != element.defaultValue;

)

In that case, I think this problem would be solved. I just tried and it looks like it will work.
I added a commit to share, can you please check if this solves the cases you are aware of?

Refactor product bulk update to normalize submitted product parameter…

eb0ea1b

…s and enhance form submission handling for changed fields

github-project-automation Bot added this to OFN Delivery board May 17, 2026

github-project-automation Bot moved this to All the things 💤 in OFN Delivery board May 17, 2026

chahmedejaz moved this from All the things 💤 to In Progress ⚙ in OFN Delivery board May 17, 2026

github-actions Bot assigned chahmedejaz May 17, 2026

chahmedejaz added 4 commits May 18, 2026 01:27

fix lint issues

58591ad

Enhance variant unit value handling during updates to support scale c…

ebbd529

…hanges

fix lint issues

9b795e4

improve test coverage

40df57e

chahmedejaz added technical changes only These pull requests do not contain user facing changes and are grouped in release notes bug-s3 The bug is stopping a critical or non-critical feature but there is a usable workaround. labels May 21, 2026

chahmedejaz marked this pull request as ready for review May 21, 2026 20:57

chahmedejaz moved this from In Progress ⚙ to Code review 🔎 in OFN Delivery board May 21, 2026

rioug approved these changes May 22, 2026

View reviewed changes

dacook self-requested a review May 26, 2026 00:42

wip: Enable change tracking on unit_value to ensure it gets saved

2aba77a

dacook requested changes May 26, 2026

View reviewed changes

github-project-automation Bot moved this from Code review 🔎 to In Progress ⚙ in OFN Delivery board May 26, 2026

	// This doesn't work with hidden field
	// Workaround: use a text field with "display:none;"
	return element.defaultValue !== undefined && element.value != element.defaultValue;

Uh oh!

Conversation

chahmedejaz commented May 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What? Why?

Solution

What should we test?

Release notes

Uh oh!

rioug left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dacook left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

chahmedejaz commented May 17, 2026 •

edited

Loading