Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions app/assets/javascripts/application_legacy.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
//= require_self
//= require big
//= require units-converter
//= require validation-utils
Copy link
Copy Markdown
Contributor

@lentschi lentschi Oct 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

//= require migrate-units-form
//= require article-form
//= require unit-conversion-field
Expand Down
43 changes: 41 additions & 2 deletions app/assets/javascripts/article-form.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ class ArticleForm {
this.supplierUnitSelect$ = $(`#${this.unitFieldsIdPrefix}_supplier_order_unit`, this.articleForm$);
this.unitRatiosTable$ = $('#fc_base_price', this.articleForm$);
this.minimumOrderQuantity$ = $(`#${this.unitFieldsIdPrefix}_minimum_order_quantity`, this.articleForm$);
this.maximumOrderQuantity$ = $(`#${this.unitFieldsIdPrefix}_maximum_order_quantity`, this.articleForm$);
this.billingUnit$ = $(`#${this.unitFieldsIdPrefix}_billing_unit`, this.articleForm$);
this.groupOrderGranularity$ = $(`#${this.unitFieldsIdPrefix}_group_order_granularity`, this.articleForm$);
this.groupOrderUnit$ = $(`#${this.unitFieldsIdPrefix}_group_order_unit`, this.articleForm$);
Expand Down Expand Up @@ -60,7 +61,7 @@ class ArticleForm {
const tax = parseFloat(this.tax$.val());
const deposit = parseFloat(this.deposit$.val());
const grossPrice = (price + deposit) * (tax / 100 + 1);
const fcPrice = grossPrice * (this.priceMarkup / 100 + 1);
const fcPrice = grossPrice * (this.priceMarkup / 100 + 1);
const priceUnitLabel = this.getUnitLabel(this.priceUnit$.val());
this.fcPrice$.find('.price_value').text(isNaN(fcPrice) ? '?' : I18n.l('currency', fcPrice));
this.fcPrice$.find('.price_per_text').toggle(priceUnitLabel.trim() !== '');
Expand Down Expand Up @@ -88,7 +89,7 @@ class ArticleForm {
this.loadRatios();
this.undoPriceConversion();
this.undoOrderAndReceivedUnitsConversion();
} catch(err) {
} catch (err) {
e.preventDefault();
throw err;
}
Expand Down Expand Up @@ -191,6 +192,7 @@ class ArticleForm {
initializeRegularFormFields() {
this.unit$.change(() => {
this.setMinimumOrderUnitDisplay();
this.setMaximumOrderUnitDisplay();
this.updateAvailableBillingAndGroupOrderUnits();
this.updateUnitMultiplierLabels();
this.updateCustomUnitWarning();
Expand All @@ -204,6 +206,28 @@ class ArticleForm {
this.updateCustomUnitWarning();
});
this.onSupplierUnitChanged();

// Add validation for minimum/maximum order quantity
this.initializeOrderQuantityValidation();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

image

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?

}

initializeOrderQuantityValidation() {
const validateOrderQuantities = () => {
const minValue = parseFloat(this.minimumOrderQuantity$.val()) || 0;
const maxValue = parseFloat(this.maximumOrderQuantity$.val()) || Infinity;

if (this.minimumOrderQuantity$.val() && this.maximumOrderQuantity$.val() && minValue > maxValue) {
const errorMessage = I18n.t('activerecord.errors.models.article_version.attributes.minimum_order_quantity.greater_than_maximum');
this.minimumOrderQuantity$[0].setCustomValidity(errorMessage);
this.maximumOrderQuantity$[0].setCustomValidity(errorMessage);
} else {
this.minimumOrderQuantity$[0].setCustomValidity('');
this.maximumOrderQuantity$[0].setCustomValidity('');
}
};

this.minimumOrderQuantity$.on('input change', validateOrderQuantities);
this.maximumOrderQuantity$.on('input change', validateOrderQuantities);
}

updateCustomUnitWarning() {
Expand All @@ -227,6 +251,7 @@ class ArticleForm {
this.unit$.toggle(!valueChosen);
this.filterAvailableRatioUnits();
this.setMinimumOrderUnitDisplay();
this.setMaximumOrderUnitDisplay();
this.updateAvailableBillingAndGroupOrderUnits();
this.updateUnitMultiplierLabels();
}
Expand All @@ -245,6 +270,20 @@ class ArticleForm {
this.minimumOrderQuantity$.attr('step', converter.isUnitSiConversible(this.supplierUnitSelect$.val()) ? 'any' : 1);
}

setMaximumOrderUnitDisplay() {
const chosenOptionLabel = this.supplierUnitSelect$.val() !== ''
? $(`option[value="${this.supplierUnitSelect$.val()}"]`, this.supplierUnitSelect$).text()
: undefined;
const unitVal = $(`#${this.unitFieldsIdPrefix}_unit`).val();
this.maximumOrderQuantity$
.parents('.input-group')
.find('.input-group-addon')
.text(chosenOptionLabel !== undefined ? chosenOptionLabel : unitVal);

const converter = this.getUnitsConverter();
this.maximumOrderQuantity$.attr('step', converter.isUnitSiConversible(this.supplierUnitSelect$.val()) ? 'any' : 1);
}

bindAddRatioButton() {
$('*[data-add-ratio]', this.articleForm$).on('click', (e) => {
e.preventDefault();
Expand Down
72 changes: 70 additions & 2 deletions app/assets/javascripts/group-order-form.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,47 @@ class GroupOrderForm {
this.groupBalance = config.groupBalance;
this.minimumBalance = config.minimumBalance;

ValidationUtils.setupCustomValidation(this.form$);

this.initializeIncreaseDecreaseButtons();
this.submitButton$.removeAttr('disabled');
this.initializeFormValidation();
}

initializeIncreaseDecreaseButtons() {
this.articleRows$.each((_, element) => this.initializeOrderArticleRow($(element)));
}

initializeFormValidation() {
this.form$.on('submit', (e) => {
// Update all validation messages first
this.articleRows$.each((_, row) => {
const row$ = $(row);
this.updateValidationMessages(row$);
});

// Check if form is valid - browser will prevent submission if invalid
if (!this.form$[0].checkValidity()) {
e.preventDefault();

// Show error messages for all invalid inputs
this.form$.find(':invalid').each((_, invalidInput) => {
const input$ = $(invalidInput);
if (input$.hasClass('goa-quantity')) {
// Trigger validation to show error message
ValidationUtils.validateNumericInput(input$);
}
});

// Focus on first invalid input
const firstInvalid = this.form$.find(':invalid').first();
if (firstInvalid.length > 0) {
firstInvalid.focus();
}
}
});
}

initializeOrderArticleRow(row$) {
const quantity$ = row$.find('.goa-quantity');
const tolerance$ = row$.find('.goa-tolerance');
Expand All @@ -36,8 +69,18 @@ class GroupOrderForm {
quantityAndTolerance$.change(() => {
this.updateMissingUnits(row$, quantity$);
this.updateBalance();
this.updateValidationMessages(row$);
});
quantityAndTolerance$.keyup(() => quantity$.trigger('change'));

// Handle validation events for quantity field only
quantity$.on('invalid', (e) => {
e.preventDefault();
e.stopPropagation();
this.updateValidationMessages(row$);
});

this.updateValidationMessages(row$);
}

updateBalance() {
Expand All @@ -59,8 +102,8 @@ class GroupOrderForm {
// determine bgcolor and submit button state according to balance
var bgcolor = '';
if (balance < this.minimumBalance) {
bgcolor = '#FF0000';
this.submitButton$.attr('disabled', 'disabled')
bgcolor = '#FF0000';
this.submitButton$.attr('disabled', 'disabled')
} else {
this.submitButton$.removeAttr('disabled')
}
Expand Down Expand Up @@ -226,5 +269,30 @@ class GroupOrderForm {
var remainder = Big(quantity).mod(packSize).toNumber();
return (remainder > 0 && (Big(remainder).add(tolerance).toNumber() >= packSize));
}

updateValidationMessages(row$) {
const quantity$ = row$.find('.goa-quantity');

if (quantity$.length === 0) return;

const inputElement = quantity$[0];

// Clear any existing validation state
inputElement.setCustomValidity('');

// Use ValidationUtils for validation with error spans
ValidationUtils.validateNumericInput(quantity$);

// Special case: item not available (max = 0)
const rawValue = quantity$.val().trim();
const inputValue = parseFloat(rawValue);
const maxValue = parseFloat(quantity$.attr('max'));

if (maxValue === 0 && inputValue > 0) {
const message = I18n.t('errors.item_not_available');
inputElement.setCustomValidity(message);
ValidationUtils.showValidationMessage(quantity$, message);
}
}
}

21 changes: 15 additions & 6 deletions app/assets/javascripts/unit-conversion-field.js
Original file line number Diff line number Diff line change
Expand Up @@ -185,13 +185,22 @@
const unitLabel = this.unitSelectOptions.find(option => option.value === unit).label;
this.conversionResult$.text('= ' + this.getConversionResult() + ' x ' + unitLabel);
this.conversionResult$.parent().find('.numeric-step-error').remove();
if (this.quantityInput$.is(':invalid')) {
this.applyButton$.attr('disabled', 'disabled');
const errorSpan$ = $(`<div class="numeric-step-error">${I18n.t('errors.step_error', {min: 0, granularity: this.quantityInput$.attr('step')})}</div>`);
errorSpan$.show();
this.conversionResult$.after(errorSpan$);
} else {

const errorSpan$ = $('<div class="numeric-step-error"></div>');

// Use ValidationUtils for proper custom error messages
const isValid = ValidationUtils.validateNumericInput(this.quantityInput$, {
errorSpan$: errorSpan$
});

if (isValid) {
this.applyButton$.removeAttr('disabled');
} else {
this.applyButton$.attr('disabled', 'disabled');
if (errorSpan$.text()) {
errorSpan$.show();
this.conversionResult$.after(errorSpan$);
}
}
}

Expand Down
155 changes: 155 additions & 0 deletions app/assets/javascripts/validation-utils.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,155 @@
// Shared validation utilities for form inputs
class ValidationUtils {
/**
* Sets up custom validation messages while keeping browser validation active
* @param {jQuery} form$ - The form element
*/
static setupCustomValidation(form$) {
if (form$.length === 0) return;

// Keep HTML5 validation active but override messages
// Don't set novalidate - we want the browser to prevent submission

// Set up custom validation for quantity inputs only
form$.find('input[type="number"].goa-quantity').each((_, input) => {
const input$ = $(input);

// Set up event handlers for custom validation messages
input$.on('invalid.customValidation', (e) => {
e.preventDefault(); // Prevent browser validation popup

// Apply our custom validation and show message in popover
ValidationUtils.validateNumericInput(input$, {
showInSpanOnly: true // Don't use setCustomValidity for display
});
});
});
}
/**
* Validates a numeric input and sets custom validation messages
* @param {jQuery} input$ - The input element to validate
* @param {Object} options - Validation options
* @returns {boolean} - Whether the input is valid
*/
static validateNumericInput(input$, options = {}) {
if (input$.length === 0) return true;

const inputElement = input$[0];
const rawValue = input$.val().trim().replace(',', '.');
const inputValue = parseFloat(rawValue);

let customMessage = '';

// Only validate if we have a valid number
if (rawValue !== '' && !isNaN(inputValue)) {
// Quantity field validation only
customMessage = ValidationUtils.validateQuantityField(input$, inputValue);
}

// Show validation message in error span (avoid popover conflicts)
ValidationUtils.showValidationMessage(input$, customMessage);

// Update visual error span if it exists (keep for backward compatibility)
if (options.errorSpan$ && options.errorSpan$.length > 0) {
if (customMessage) {
options.errorSpan$.text(customMessage).show();
} else {
options.errorSpan$.hide();
}
}

// Set custom validation message for browser validation
// This will be used by the browser to prevent form submission
if (!options.showInSpanOnly) {
inputElement.setCustomValidity(customMessage);
}

// Return validation state
return customMessage === '';
}

/**
* Validates a quantity field
* @param {jQuery} input$ - The quantity input element
* @param {number} inputValue - The parsed input value
* @returns {string} - Error message or empty string if valid
*/
static validateQuantityField(input$, inputValue) {
// Get validation constraints from HTML attributes
const maxValue = input$.attr('max') ? parseFloat(input$.attr('max')) : null;
const minValue = parseFloat(input$.attr('min')) || 0;
const stepValue = parseFloat(input$.attr('step')) || 1;

// Check maximum quantity first (highest priority)
if (maxValue !== null && inputValue > maxValue) {
return I18n.t('errors.maximum_quantity_error', { max: maxValue });
}
// Check minimum value
else if (inputValue < minValue) {
return I18n.t('errors.step_error', { min: minValue, granularity: stepValue });
}
// Check step/granularity (allow for floating point precision issues)
else if (stepValue > 0) {
const remainder = ((inputValue - minValue) % stepValue);
if (Math.abs(remainder) > 0.0001 && Math.abs(remainder - stepValue) > 0.0001) {
return I18n.t('errors.step_error', { min: minValue, granularity: stepValue });
}
}

return '';
}

/**
* Shows or hides validation message using error spans (avoids popover conflicts)
* @param {jQuery} input$ - The input element
* @param {string} message - The validation message (empty to hide)
*/
static showValidationMessage(input$, message) {
if (input$.length === 0) return;

// Find the error span for this input
const errorSpan$ = input$.closest('.group-order-input').find('.numeric-step-error');

if (message) {
// Show error message in span and add error styling
if (errorSpan$.length > 0) {
errorSpan$.text(message).show();
}
input$.addClass('validation-error');
} else {
// Hide error message and remove styling
if (errorSpan$.length > 0) {
errorSpan$.hide();
}
input$.removeClass('validation-error');
}
}

/**
* Hides all validation messages
*/
static hideAllValidationMessages() {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Method seems to be unused - AI slop?

$('.validation-error').removeClass('validation-error');
$('.numeric-step-error').hide();
}



/**
* Sets up validation for a form input
* @param {jQuery} input$ - The input element
* @param {Object} options - Validation options
*/
static setupInputValidation(input$, options = {}) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Method seems to be unused - AI slop?

if (input$.length === 0) return;

// Only set up event handlers, don't trigger validation immediately
// to avoid recursion when called from existing event handlers
input$.on('invalid.validationUtils', (e) => {
e.preventDefault();
e.stopPropagation();
ValidationUtils.validateNumericInput(input$, options);
return false;
});
}
}
1 change: 1 addition & 0 deletions app/assets/stylesheets/application.scss
Original file line number Diff line number Diff line change
Expand Up @@ -11,3 +11,4 @@
@import "list.missing";
@import "recurring_select";
@import "actiontext";
@import "validation";
Loading
Loading