feat: maximumx order quantity for order article by yksflip · Pull Request #1201 · foodcoops/foodsoft

yksflip · 2025-07-09T10:10:16Z

implements #1007

lentschi · 2025-07-12T08:55:08Z

@yksflip Just to make sure before looking at it in depth - is this ready for review? (If not, please mark the PR as draft. 😉 )

I tried it out locally and got an unhandled error right away:

NoMethodError (undefined method `quantity' for nil:NilClass):
  
app/models/group_order.rb:67:in `block (2 levels) in load_data'
app/models/group_order.rb:41:in `each'
app/models/group_order.rb:41:in `block in load_data'
app/models/group_order.rb:40:in `each'
app/models/group_order.rb:40:in `load_data'
app/controllers/group_orders_controller.rb:23:in `new'
app/controllers/application_controller.rb:92:in `set_currency'
app/controllers/application_controller.rb:80:in `set_time_zone'

All I did to get there was create a new order including an article with a maximum_order_quantity and then try to open the new group order form for that order.

Also, I think there's (at least) some logic missing in app/assets/javascripts/article-form.js and app/assets/javascripts/group-order-form.js - they both would need to handle the new field similar to the current logic for minimum_order_quantity, right? (If you need help with the clunky jquery, let me know! It's really about time we tackle #1194 😅 )

yksflip · 2025-07-15T20:57:04Z

thanks lentschi! I thought it would be ready, but there are few loops to take. I'll mark it a draft and ping you again once I'm ready :)

yksflip · 2025-07-15T21:47:31Z

I tried it out locally and got an unhandled error right away:

I fixed it by adding a null check.

Also, I think there's (at least) some logic missing in app/assets/javascripts/article-form.js

Indeed! I've added it almost identitcal to max-quantity ..

and app/assets/javascripts/group-order-form.js

I think that is not needed as I used the quantity_available in group_order.rb ( https://github.com/foodcoops/foodsoft/pull/1201/files#diff-b946eb642b5614e4a0519a74fc97aec4c0bcec98db9c99c790ea7c048349b5acR63 ) that was already implemented for the stock-order feature I think

lentschi · 2025-07-24T15:33:45Z

I think that is not needed as I used the quantity_available in group_order.rb ( https://github.com/foodcoops/foodsoft/pull/1201/files#diff-b946eb642b5614e4a0519a74fc97aec4c0bcec98db9c99c790ea7c048349b5acR63 ) that was already implemented for the stock-order feature I think

Ah, right, seems to work now 👍

But maybe we should also change the error message when entering an invalid value manually (exceeding max):

(It now only mentions min and granularity)

I know, you could argue, that this has never been in there, but quantity_available has so far only been checked for stock orders and those have always been quirky and aren't widely used. 😅

lentschi · 2025-07-24T15:35:05Z

Also, I think there's (at least) some logic missing in app/assets/javascripts/article-form.js

Indeed! I've added it almost identitcal to max-quantity ..

👍
Maybe we should also add a validation that minimum_order_quantity must be <= maximum_order_quantity?

yksflip · 2025-10-14T13:54:50Z

I've added backend and client side validations for this now.
The javascript client-side validation were quite complicated, good old friend ai helped a little bit out here. Not sure if it needs to be a custom validation like this. see app/assets/javascripts/validation-utils.js
maybe there is a better / more idiomatic way to do this?

lentschi

I've added backend and client side validations for this now.
The javascript client-side validation were quite complicated, good old friend ai helped a little bit out here. Not sure if it needs to be a custom validation like this. see app/assets/javascripts/validation-utils.js
maybe there is a better / more idiomatic way to do this?

I've added some code comments.

In addition, two general issues I'd like to throw in:

Currently when two users open the group order form at the same time, then one adds a quantity that is below maximum_order_quantity and the second user tries the same but it's exceeding maximum_order_quantity when combined with the first user's quantity, the second user just gets a "The order couldn’t be updated due to a bug." So maybe add some custom error flash here if the maximum_order_quantity is exceeded? (It would be better to have the form error inside the form, but I guess that's more difficult to implement.)
I haven't tried, but have you checked, if "using up" tolerance, can't be causing to somehow exceed maximum_order_quantity?

lentschi · 2025-10-18T11:07:39Z

    this.onSupplierUnitChanged();
+
+    // Add validation for minimum/maximum order quantity
+    this.initializeOrderQuantityValidation();


Why did you add client side validation to this form? I think, since other form fields are validated by the backend, this makes this kind of inconsistent. If I remove that line, it works just as fine - after clicking "Update article", I see the following error:

Of course frontend validation is always a nice-to-have in terms of usability, as the users can be made aware as soon as they blur the erroneous field. But I was hoping to only add this when finally building a PWA style client and not to further complicate the already ugly jquery code. What do you think?

lentschi · 2025-10-18T15:23:34Z

+     * @param {jQuery} input$ - The input element
+     * @param {Object} options - Validation options
+     */
+    static setupInputValidation(input$, options = {}) {


Method seems to be unused - AI slop?

lentschi · 2025-10-18T15:23:56Z

+    /**
+     * Hides all validation messages
+     */
+    static hideAllValidationMessages() {


Method seems to be unused - AI slop?

lentschi · 2025-10-18T15:29:11Z

-
+  validates :maximum_order_quantity, numericality: { allow_nil: true }
+  # validates_uniqueness_of :name, :scope => [:supplier_id, :deleted_at, :type], if: Proc.new {|a| a.supplier.shared_sync_method.blank? or a.supplier.shared_sync_method == 'import' }
+  # validates_uniqueness_of :name, :scope => [:supplier_id, :deleted_at, :type, :unit, :unit_quantity]


It's odd that this is shown as part of a change although it's the same as in the current master - maybe due to a faulty rebase...?

lentschi · 2025-10-18T16:02:08Z

            - quantity_data['used_quantity'] = @ordering_data[:order_articles][order_article.id][:used_quantity]
            - quantity_data['price'] = @ordering_data[:order_articles][order_article.id][:price]
            - quantity_data['minimum_order_quantity'] = @ordering_data[:order_articles][order_article.id][:minimum_order_quantity] unless @ordering_data[:order_articles][order_article.id][:minimum_order_quantity].nil?
+            - quantity_data['maximum_order_quantity'] = @ordering_data[:order_articles][order_article.id][:maximum_order_quantity] unless @ordering_data[:order_articles][order_article.id][:maximum_order_quantity].nil?


If I'm not mistaken, @ordering_data[:order_articles][order_article.id][:maximum_order_quantity] is never set, right? (At least the above fields are all set in GroupOrder.load_data, but maximum_order_quantity isn't.)

Anyhow - quantity_available seems sufficient, right?

lentschi · 2025-10-18T17:14:05Z

 //= require_self
 //= require big
 //= require units-converter
+//= require validation-utils


I think the AI added some unused methods for these validation-utils (I added review comments for where I seem to have found dead code) and also the solution it provided supersedes some old logic, which it also failed to remove (e.g. the %span.numeric-step-error in the group order form template no longer seems to be required).

Instead of reviewing the AI code in depth (which seems cumbersome, as I think there's quite some AI slop), here's my quick go on this:

https://github.com/foodcoops/foodsoft/pull/1232/files#diff-ade87a685fd4a0e008033752780385cce4bbfff5d1355fab4b35cafffc2b3080R44

Instead of reviewing the AI code in depth (which seems cumbersome, as I think there's quite some AI slop), here's my quick go on this:

https://github.com/foodcoops/foodsoft/pull/1232/files#diff-ade87a685fd4a0e008033752780385cce4bbfff5d1355fab4b35cafffc2b3080R44

<- @yksflip As there hasn't been any activity on this PR for a while, I have closed my PR on top of this. But I can reopen as soon as you or someone else wants pick up on it.

yksflip · 2025-11-06T15:42:08Z

thanks for your comments and your approach to the client-validations! I should have better checked the generated stuff... I'll integrate your PR next and then tackle your comments :)

Currently when two users open the group order form at the same time, then one adds a quantity that is below maximum_order_quantity and the second user tries the same but it's exceeding maximum_order_quantity when combined with the first user's quantity, the second user just gets a "The order couldn’t be updated due to a bug." So maybe add some custom error flash here if the maximum_order_quantity is exceeded? (It would be better to have the form error inside the form, but I guess that's more difficult to implement.)

so this is the server side validation that doesn't return a nice error message right? I'll try to fix it.

I haven't tried, but have you checked, if "using up" tolerance, can't be causing to somehow exceed maximum_order_quantity?

I think this shouldn't happen as long as minimum < maximum, as you can't order more than the minimum with the tolerance field. But I'll give this a few more testcases just to be sure

lentschi · 2025-11-06T16:03:17Z

so this is the server side validation that doesn't return a nice error message right? I'll try to fix it.

Yes, exactly. 👍

I think this shouldn't happen as long as minimum < maximum, as you can't order more than the minimum with the tolerance field.

Okay, great - just checking :)

I'll integrate your PR next and then tackle your comments :)

👍

yksflip force-pushed the feat/1007-max-order-quantity branch 3 times, most recently from fb2f5d6 to 2f32e39 Compare July 9, 2025 10:42

yksflip marked this pull request as draft July 15, 2025 20:57

yksflip force-pushed the feat/1007-max-order-quantity branch from 2f32e39 to b48c721 Compare July 15, 2025 21:42

yksflip force-pushed the feat/1007-max-order-quantity branch 2 times, most recently from a02df13 to a00843d Compare October 14, 2025 13:51

feat: maximum order quantity

8f043e4

yksflip force-pushed the feat/1007-max-order-quantity branch from a00843d to 8f043e4 Compare October 14, 2025 13:56

yksflip marked this pull request as ready for review October 14, 2025 13:58

lentschi requested changes Oct 18, 2025

View reviewed changes

lentschi mentioned this pull request Nov 6, 2025

On #1007 AI code alternative #1232

Closed

Conversation

yksflip commented Jul 9, 2025

Uh oh!

lentschi commented Jul 12, 2025

Uh oh!

yksflip commented Jul 15, 2025

Uh oh!

yksflip commented Jul 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lentschi commented Jul 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lentschi commented Jul 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yksflip commented Oct 14, 2025

Uh oh!

lentschi left a comment

Choose a reason for hiding this comment

Uh oh!

lentschi Oct 18, 2025

Choose a reason for hiding this comment

Uh oh!

lentschi Oct 18, 2025

Choose a reason for hiding this comment

Uh oh!

lentschi Oct 18, 2025

Choose a reason for hiding this comment

Uh oh!

lentschi Oct 18, 2025

Choose a reason for hiding this comment

Uh oh!

lentschi Oct 18, 2025

Choose a reason for hiding this comment

Uh oh!

lentschi Oct 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lentschi Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

yksflip commented Nov 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lentschi commented Nov 6, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

yksflip commented Jul 15, 2025 •

edited

Loading

lentschi commented Jul 24, 2025 •

edited

Loading

lentschi commented Jul 24, 2025 •

edited

Loading

lentschi Oct 18, 2025 •

edited

Loading

yksflip commented Nov 6, 2025 •

edited

Loading