[IP-841]: Copy Quote UI feedback #932

Verony-makesIT · 2023-02-21T22:07:17Z

Description

The ajax form doesn't post because there is no response from the backend.
The problem comes when fetching the 'custom_fields' data.
As far as I understand it, the ( existing?) data is retrieved from the custom_fields db and put into an array.
If there is no data in the db the array cannot be populated and as a result of this there is no "response".
Therefore, in modal_copy_quote, the code

if (response.success === 1) {
    window.location = "<?php echo site_url('invoices/view'); ?>/" + response.invoice_id;
}

is not executed in order to close the modal window and to display the copied quote view page.
To solve the problem I extracted the snippet (copied the custom_fields code) from mdl_invoice.php and modified it in mdl_quotes.php.

Related Issue

#841 Copy quote (modal) UI feedback

Motivation and Context

#841 See description

Bugfix: Selected quote date is saved
See commits
Bugfix: Other client can be selected from list
See commits

Screenshots (if appropriate):

Pull Request Checklist

My code follows the code formatting guidelines.
I have an issue ID for this pull request.
I selected the corresponding branch.
I have rebased my changes on top of the corresponding branch.

Issue Type (Please check one or more)

Bugfix
Improvement of an existing Feature
New Feature

Bug: No other client can be selected/looked up from the client list. Only the customer from the source invoice was selected. Fix: Selecting an other client is now possible. The selection is saved in the copied invoice.

Bug: The quote date from the source quote was saved and not the selected or entered date from the datepicker. The original quote date was always saved in the copied quote. Fix: The selected date from the datepicker is saved in the copied quote.

Bug: When the data is retrieved from the custom_fields db and put into an array and if there is no data in the db, the array cannot be populated so the result is that there is no "response" at all. Fix: extracted the working snippet from mdl_invoice and modified it in mdl_quotes. Even when there is no data/array there is a response.

Verony-makesIT · 2023-02-25T10:02:37Z

@nielsdrost7:
I have read your comments in slack (comment 1 and comment 2) and replied to them in slack (my answer) as well.
I regret that my proposal (PR) to fix these bugs is now temporarily (?) put "on hold" while I think there is a misunderstanding.
The additional bug (see screenshots) occurs in "Copy Invoice" and not in "Copy Quote".
Commit:

Bug (in Copy Invoice):

@nielsdrost7, @naui95, @clockwiseq, ...?
After reading my comments in slack, can any of the moderators, reviewers clarify to me what else is expected of me?
The most important thing, in my opinion, is that issues in IP be fixed asap.....

naui95 · 2023-02-25T10:21:28Z

Dear @Verony-makesIT thank you for the further explanations. Your PR AFAIK is not on hold but just pending review. I personally still did not have time to check out the changes and to test them (and eventually get back to you with any question I might have). Please consider that an update has already been planned and at the moment this fix is not included in the upcoming update (see https://github.com/InvoicePlane/InvoicePlane/releases and https://github.com/InvoicePlane/InvoicePlane/milestone/10), which means the review and integration of this PR is - in my opinion - not top priority at the moment. We really appreciate your contributions to the IP community and code base and fully agree with the idea that issues must be fixed.

nielsdrost7 · 2023-02-25T15:14:51Z

Same here: I haven't had time to take a look at your PR, @Verony-makesIT
We appreciate your PR's, but you're going to have to wait some more until we have time to take a look at it

naui95 · 2024-01-26T13:36:20Z

@Verony-makesIT can the PR be considered finished or is it still a draft?

naui95 · 2024-01-26T13:39:45Z

closes #841
closes #1019

Verony-makesIT · 2024-01-29T15:45:19Z

@naui95,
I just did some testing to see if Copy Quote and Copy Invoice were still working correctly and everything seems to be fine for me.
So the PR can be considered finished.

naui95

Hey, thank you for the PR. The PR works well 👍 there are some changes to be made in my opinion; I've commented the relevant code lines. Some changes go too far and I would revert them to the original solution.

The PR solves more bugs than the original issue, in particular solves:

feedback on quote copied (needs changes before merging)
fixes a bug affecting the ability to select an other client when creating a copy of an invoice
fixes a bug affecting the date chosen by the modal post when copying a quote

For future reference: it would be better to make different issues and PRs for different bugs.

naui95 · 2024-03-10T07:51:26Z

application/modules/quotes/models/Mdl_quotes.php

@@ -217,13 +217,13 @@ public function copy_quote($source_id, $target_id)

        // Copy the custom fields
        $this->load->model('custom_fields/mdl_quote_custom');
-        $db_array = $this->mdl_quote_custom->where('quote_id', $source_id)->get()->row_array();
+        $custom_fields = $this->mdl_quote_custom->where('quote_id', $source_id)->get()->result();


result() method does not achieve the same as row_array() / row() (see: https://codeigniter.com/userguide3/database/results.html). I am not sure this is right.

The main issue of this PR is caused by the fact that the $db_array variable can be null and as a consequence the count() function at row 222 fails. A clean solution to the matter could be to simply avoid having a null return by editing row 220 as follows:
$db_array = $this->mdl_quote_custom->where('quote_id', $source_id)->get()->row_array() ?? [];

The above mentioned solution would allow us not to change how the fields are then copied in a quote (avoiding to have to test the new solution).

@naui95, because I wanted to use the copy quotes function in IP I discovered this problem. Although I am absolutely no programmer I tried to solve this bug by comparing this "copy quotes" function with the "copy invoices" function.
I succeeded by copying the piece of code from the "copy invoices" function.
Your proposal certainly seems to me to be a better solution programmatically.
Therefore, isn't it better and desirable to modify your proposal/solution in the "copy invoices" function as well?

Hi @Verony-makesIT thank you 🙏. Yes, you might be right. I did not look at how the copying with invoices is done. I will check it out this weekend. If the logic should be the same it might be wise to adapt also the copy invoice as you are suggesting. I will let you know.

naui95 · 2024-03-10T08:25:46Z

application/modules/quotes/views/modal_copy_quote.php

                           class="form-control datepicker"
-                           value="<?php echo date_from_mysql($quote->quote_date_created, true); ?>">
+                           value="<?php echo date_from_mysql(date('Y-m-d', time()), true); ?>">


I think this should be discussed. Here we are changing the original behavior which was to suggest as creation date the original quote date. I believe this is over the changes expected with the resolution of the issue. I suggest the change is reverted.

@naui95, I agree that this should be bediscussed.
I have no problem reverting this change but...
If I find an "old quote/invoice" with the same items as the one I need then I will use this copy functionality provided I only need to change some limited items.
Anyway, I will usually always change the old quotation/invoice date to the current date.
In my opinion it makes no sense to keep that old date.
Just my 2 cents.

I leave it up to you to decide.

@Verony-makesIT you actually have a point (or more than one). The behavior you are suggesting is also followed when copying invoices and I suppose is even the expected one.

InvoicePlane/application/modules/invoices/views/modal_copy_invoice.php

Line 70 in bd22ac6

value="<?php echo date_from_mysql(date('Y-m-d', time()), true) ?>">

My comment is in fact narrow minded, because in principle a PR should only solve the bug/enhancement/etc. to which it is related. This PR solves multiple bugs (thank you!), not all referenced by the issue, and when I went through your code this change did not look as referenced to a bug to me and...therefore the revert suggestion.

Let's see what @nielsdrost7 thinks about merging this change. I believe that it might be wise to keep it in, as @Verony-makesIT pointed out.

@mheiduk Is this another one of those situations where the date suddenly changes? Just wondering.

naui95 · 2024-03-16T13:28:34Z

application/modules/quotes/models/Mdl_quotes.php

-        if (count($db_array) > 2) {
-            unset($db_array['quote_custom_id']);
-            $db_array['quote_id'] = $target_id;
-            $this->mdl_quote_custom->save_custom($target_id, $db_array);
-        }


@Verony-makesIT I think this part is also necessary and should be included. Am I missing something?

@naui95 As I've tried to make clear before, I'm absolutely not a programmer and in this case I simply looked at the same function in Mdl_Invoices.php, then copied this code into Mdl_Quotes.php and tested it.
Everything worked then without any problems and so I assumed that this is/was (?) the solution for this bug.
Sorry, but I have to admit that I don't have enough (developer) knowledge to answer this question.
I could be completely wrong, but in my humble opinion this code is not necessary here because it also works in Mdl_Invoices.php, in the same way and without the "if (count($db_array) > 2){ }" code.

nielsdrost7 · 2024-03-16T15:01:15Z

application/modules/quotes/models/Mdl_quotes.php

-        }
-        $this->mdl_quote_custom->save_custom($target_id, $form_data);
+        $db_array = $this->mdl_quote_custom->where('quote_id', $source_id)->get()->row_array() ?? [];
+        $this->mdl_quote_custom->save_custom($target_id, $db_array);


If it is an empty array [] you're still saving that empty array to the custom_fields for the quote?
Why?

@nielsdrost7 I think we have the same question. I believe that a slice of the original code was accidentally left out (see: #932 (comment))

@naui95 My apologies.
Here I indeed made a stupid mistake and accidentally modified the code incorrectly.
In a moment I will restore the code (as I had previously modified it)...

nielsdrost7 · 2024-03-17T00:23:31Z

@Verony-makesIT thank you for your PR.
A couple of general things:

Normally, don't solve multiple bugs in 1 PR
Please don't change 'other' things in the same PR as you're solving bugs in.

Right now you're solving bug #932 and feature #1019, right? OK, solved, move on.
Then you're solving something about a client not being set, correct? OK, great, the issue was "copy quote feedback", fine, let's move on.

But wait a minute ... you're also changing a date to today's date after copying the quote?
And that was something that needed to be discussed?

So now we need to iron out 1 thing that had absolutely nothing to do with solving some bugs? ai, ai.

nielsdrost7 · 2024-03-17T00:28:10Z

One last thing, before u have to go over the source itself (indenting problems in the translation file?)

Could you take a look at naui's and my remarks regarding #932 (comment) ?
As soon as some piece of code 'disappears' some bugs may pop up.
I think naui suggested some extra code was needed, focus on that please.
My remark regarding saving the empty array can easily be resolved, but do that after you've solve the other things

application/modules/invoices/models/Mdl_invoices.php

nielsdrost7 · 2024-03-24T09:29:39Z

@Verony-makesIT I'll try to make a checklist for you to go over, before we can check the PR one more time and then merge the solution for the Copy Quote UI Feedback:

https://github.com/InvoicePlane/InvoicePlane/pull/932/files#r1518791594 : very important. When someone makes a comment and then requests changes we cannot merge until those changes have been resolved. I think that single change you made has to be reverted. I can't see which commit it is, but it should be something like git revert abc13455bca
Fix all the other remarks from naui95, for example here: https://github.com/InvoicePlane/InvoicePlane/pull/932/files#r1527185594 I believe that a slice of the original code was accidentally left out (see: #932 (comment))
Last but not least: https://github.com/InvoicePlane/InvoicePlane/pull/932/files/7e34967e66f5b62ee34700e34a3a8921271fe3b8#r1528110936 : Remove an "Enter" in a file, since we're changing that file anyway because of your PR, we can remove that "Enter"

Let me know once you're done with these changes, then we can make our final checks.

Verony-makesIT added 3 commits February 21, 2023 20:49

Bugfix: Other client can be selected from list

1ee6c6d

Bug: No other client can be selected/looked up from the client list. Only the customer from the source invoice was selected. Fix: Selecting an other client is now possible. The selection is saved in the copied invoice.

Bugfix: Selected quote date is saved

6adef22

Bug: The quote date from the source quote was saved and not the selected or entered date from the datepicker. The original quote date was always saved in the copied quote. Fix: The selected date from the datepicker is saved in the copied quote.

nielsdrost7 changed the title ~~Upstream/bugfix/ip 841~~ Copy Quote UI feedback Feb 22, 2023

nielsdrost7 changed the title ~~Copy Quote UI feedback~~ [IP-841] Copy Quote UI feedback Feb 22, 2023

Verony-makesIT added 2 commits May 1, 2023 18:14

small format indent issue

d62fc9e

Merge branch 'InvoicePlane:development' into upstream/bugfix/IP-841

382a787

nielsdrost7 changed the title ~~[IP-841] Copy Quote UI feedback~~ [IP-841]: Copy Quote UI feedback Dec 23, 2023

nielsdrost7 marked this pull request as draft December 23, 2023 09:40

nielsdrost7 added this to the 1.6.2 milestone Dec 23, 2023

Verony-makesIT added 2 commits December 24, 2023 12:54

Merge branch 'InvoicePlane:development' into upstream/bugfix/IP-841

2de7010

Merge branch 'InvoicePlane:development' into upstream/bugfix/IP-841

7e34967

Verony-makesIT mentioned this pull request Jan 26, 2024

[Feature Request] copy quote needs confirmation popup model box #1019

Open

naui95 self-requested a review January 26, 2024 13:32

naui95 self-assigned this Jan 26, 2024

naui95 marked this pull request as ready for review January 29, 2024 19:39

naui95 requested changes Mar 10, 2024

View reviewed changes

Adapt copy custom fields code if null

7d0e492

naui95 reviewed Mar 16, 2024

View reviewed changes

nielsdrost7 reviewed Mar 16, 2024

View reviewed changes

nielsdrost7 reviewed Mar 18, 2024

View reviewed changes

application/modules/invoices/models/Mdl_invoices.php Show resolved Hide resolved

nielsdrost7 marked this pull request as draft March 24, 2024 09:29

Revert some accidently changes

a4a0d54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[IP-841]: Copy Quote UI feedback #932

[IP-841]: Copy Quote UI feedback #932

Verony-makesIT commented Feb 21, 2023

Verony-makesIT commented Feb 25, 2023

naui95 commented Feb 25, 2023

nielsdrost7 commented Feb 25, 2023

naui95 commented Jan 26, 2024

naui95 commented Jan 26, 2024

Verony-makesIT commented Jan 29, 2024

naui95 left a comment

naui95 Mar 10, 2024

Verony-makesIT Mar 11, 2024

naui95 Mar 12, 2024

naui95 Mar 10, 2024 •

edited

Verony-makesIT Mar 11, 2024

naui95 Mar 12, 2024

nielsdrost7 Mar 17, 2024

naui95 Mar 16, 2024

Verony-makesIT Mar 30, 2024

nielsdrost7 Mar 16, 2024

naui95 Mar 16, 2024

Verony-makesIT Mar 30, 2024

nielsdrost7 commented Mar 17, 2024

nielsdrost7 commented Mar 17, 2024

nielsdrost7 commented Mar 24, 2024 •

edited by naui95

[IP-841]: Copy Quote UI feedback #932

Are you sure you want to change the base?

[IP-841]: Copy Quote UI feedback #932

Conversation

Verony-makesIT commented Feb 21, 2023

Description

Related Issue

Motivation and Context

Screenshots (if appropriate):

Pull Request Checklist

Issue Type (Please check one or more)

Verony-makesIT commented Feb 25, 2023

naui95 commented Feb 25, 2023

nielsdrost7 commented Feb 25, 2023

naui95 commented Jan 26, 2024

naui95 commented Jan 26, 2024

Verony-makesIT commented Jan 29, 2024

naui95 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

naui95 Mar 10, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nielsdrost7 commented Mar 17, 2024

nielsdrost7 commented Mar 17, 2024

nielsdrost7 commented Mar 24, 2024 • edited by naui95

naui95 Mar 10, 2024 •

edited

nielsdrost7 commented Mar 24, 2024 •

edited by naui95