feat: maximumx order quantity for order article #1201

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

app/assets/javascripts/article-form.js

    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

app/assets/javascripts/validation-utils.js

+     * @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

app/assets/javascripts/validation-utils.js

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


Method seems to be unused - AI slop?

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

app/models/article_version.rb

-
+  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

app/views/group_orders/_form.html.haml

            - 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

app/assets/javascripts/application_legacy.js

 //= 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

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

feat: maximumx order quantity for order article #1201

Are you sure you want to change the base?

feat: maximumx order quantity for order article #1201

Uh oh!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 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