Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Show default Invoice/Quote item qty will be saved if empty #347

Conversation

thicolares
Copy link
Contributor

@thicolares thicolares commented Oct 23, 2020

Context

The bug reported in #303 is reproducible on version 2.0.4 -- run via this image tag.
But I cannot reproduce on version 2.1.0-dev: it doesn't override the Invoice Item Qty to 1 on the Edit form.
However, it doesn't show the default value anymore.

Probably reason for the bug reported on #303:

        $builder->add(
            'qty',
            NumberType::class,
            [
                'data' => 1, // <<<<<=====
                'attr' => [
                    'class' => 'input-mini quote-item-qty',
                ],
            ]
        );

The data option always overrides the value taken from the domain data (object) when rendering. This means the object value is also overridden when the form edits an already persisted object, causing it to lose its persisted value when the form is submitted. See more: https://symfony.com/doc/4.4/reference/forms/types/form.html#data

The previous fix:

        $builder->add(
            'qty',
            NumberType::class,
            [
                'empty_data' => 1, // <<<<<=====
                'attr' => [
                    'class' => 'input-mini quote-item-qty',
                ],
            ]
        );

That's the value in case the input is empty BUT we don't show the default value on the form. See more: https://symfony.com/doc/4.4/reference/forms/types/form.html#empty-data

This PR:

  • Explains what happened.
  • Displays the default value again keeping the expected behaviour.

Create an invoice (same for Quote):

image

Edit invoice (same for Quote):

image

@pierredup
Copy link
Member

Thank you for this change @thicolares. Unfortunately, I don't think it solves the issue completely.

On a new invoice, if you don't add any qty, but you type an amount, then the totals don't get updated. So even though there is a 1 displayed in the qty field, it isn't picked up when calculating the totals (although this won't be an issue when saving the invoice, just when viewing).

I did some digging, and the below patch would fix the issue.

diff --git a/src/CoreBundle/Resources/public/js/billing/view/item_row.js b/src/CoreBundle/Resources/public/js/billing/view/item_row.js
index 7544dce8..767fe9db 100644
--- a/src/CoreBundle/Resources/public/js/billing/view/item_row.js
+++ b/src/CoreBundle/Resources/public/js/billing/view/item_row.js
@@ -21,7 +21,7 @@ export default ItemView.extend({

         this.model.collection.remove(this.model);
     },
-    async initialize () {
+    async onRender () {
         await this.setModel();
         await this.calcPrice();
     },

There is already some code to set the default value to 1 if there is no value set but seems it doesn't run when the JS view is initialised. So this change would fire the code after the view has been rendered, to ensure all the values are picked up properly.

@thicolares
Copy link
Contributor Author

thicolares commented Oct 23, 2020

@pierredup you're right. I dig a bit the JS layer, but it was not clear. I'll apply this patch and test. But still, we could keep the placeholder, because this is what happens when we leave the input empty.

@pierredup
Copy link
Member

we could keep the placeholder, because this is what happens when we leave the input empty.

The JS will populate the value to 1, so the placeholder won't ever be shown (unless you clear the value).

@thicolares
Copy link
Contributor Author

thicolares commented Oct 24, 2020

The JS will populate the value to 1, so the placeholder won't ever be shown (unless you clear the value).

Exactly, I'm aware of that. I mean: that's a way to communicate the fallback: "hey, if you clear this, I'll still save it as 1" -- and that's how it behaves. Ironically because users tend to believe placeholders are default (pre-filled) values 😄 : https://www.uxmatters.com/mt/archives/2010/03/dont-put-hints-inside-text-boxes-in-web-forms.php (old, but gold)

@thicolares
Copy link
Contributor Author

@pierredup I have tested too and it works as expected. Thanks for this tip!

@pierredup
Copy link
Member

"hey, if you clear this, I'll still save it as 1" -- and that's how it behaves

I like this idea, but it doesn't seem like this is how it works currently. If I clear the value and try to save the invoice, then I get a validation error that the value is not valid.

Screenshot 2020-10-25 at 07 24 54

@thicolares
Copy link
Contributor Author

thicolares commented Oct 26, 2020

@pierredup, oh you're right. I had tested that at the beginning of my investigation with the old version 2.0.4. I noticed that validation after but simply forgot. I have just rebased it. Sorry about that. I have left the empty_data anyway as a fallback.

@thicolares thicolares force-pushed the improve-invoice-quote-qty-default-value branch from c260eda to 4421467 Compare October 26, 2020 23:38
@pierredup
Copy link
Member

Thank you @thicolares!

@pierredup pierredup merged commit d818819 into SolidInvoice:master Oct 27, 2020
@pierredup pierredup mentioned this pull request Dec 18, 2020
@pierredup pierredup added this to the 2.1.0 milestone Dec 22, 2020
@pierredup pierredup added this to In progress in Release 2.1 via automation Dec 22, 2020
@pierredup pierredup moved this from In progress to Done in Release 2.1 Mar 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Release 2.1
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants