Skip to content

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

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

Merged

Conversation

Verony-makesIT
Copy link
Contributor

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

@nielsdrost7 nielsdrost7 changed the title Upstream/bugfix/ip 841 Copy Quote UI feedback Feb 22, 2023
@nielsdrost7 nielsdrost7 changed the title Copy Quote UI feedback [IP-841] Copy Quote UI feedback Feb 22, 2023
@Verony-makesIT
Copy link
Contributor Author

@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:
image

Bug (in Copy Invoice):
image

@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
Copy link
Contributor

naui95 commented Feb 25, 2023

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
Copy link
Contributor

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

@nielsdrost7 nielsdrost7 changed the title [IP-841] Copy Quote UI feedback [IP-841]: Copy Quote UI feedback Dec 23, 2023
@nielsdrost7 nielsdrost7 marked this pull request as draft December 23, 2023 09:40
@nielsdrost7 nielsdrost7 added this to the 1.6.2 milestone Dec 23, 2023
@naui95 naui95 self-requested a review January 26, 2024 13:32
@naui95 naui95 self-assigned this Jan 26, 2024
@naui95
Copy link
Contributor

naui95 commented Jan 26, 2024

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

@naui95
Copy link
Contributor

naui95 commented Jan 26, 2024

closes #841
closes #1019

@Verony-makesIT
Copy link
Contributor Author

@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 naui95 marked this pull request as ready for review January 29, 2024 19:39
Copy link
Contributor

@naui95 naui95 left a comment

Choose a reason for hiding this comment

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

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.

@nielsdrost7
Copy link
Contributor

@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
Copy link
Contributor

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

@nielsdrost7
Copy link
Contributor

nielsdrost7 commented Mar 24, 2024

@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:

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

@nielsdrost7 nielsdrost7 marked this pull request as draft March 24, 2024 09:29
@nielsdrost7 nielsdrost7 linked an issue Sep 28, 2024 that may be closed by this pull request
Verony-makesIT and others added 6 commits September 29, 2024 13:59
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.
@naui95 naui95 force-pushed the upstream/bugfix/IP-841 branch from bd5fd3c to b681d40 Compare September 29, 2024 12:09
@naui95 naui95 marked this pull request as ready for review September 29, 2024 12:09
@naui95 naui95 self-requested a review September 29, 2024 12:10
Copy link
Contributor

@naui95 naui95 left a comment

Choose a reason for hiding this comment

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

Fixed copying as proposed and solved conflicts. Ready to go.

@naui95 naui95 merged commit 62b690e into InvoicePlane:development Sep 29, 2024
1 check passed
nielsdrost7 added a commit that referenced this pull request Dec 30, 2024
## New Contributors
Huge thanks to @sudwebdesign , @xam-ps  and @AeroBytesNL for helping. Without you guys this release wouldn't have been possible

* @sudwebdesign made their first contribution in #1149
* @VizardAlpha made their first contribution in #1129
* @pumpi made their first contribution in #1079
* @jmclaren7 made their first contribution in #1013
* @RobiNN1 made their first contribution in #1014
* @xeruf made their first contribution in #1061
* @NiklasSchmitt made their first contribution in #1073
* @redxtech made their first contribution in #1098

## What's Changed
* Add buttons on client view to delete note (Ajax) by @sudwebdesign in #1149
* Translate Client "Extra Field Title" to complete #1003 by @sudwebdesign in #1150
* [IP-1146]: after posting a Payments Form when amount > inv. total by @sudwebdesign in #1151
* [IP-1147]: Fix setup sql filenames by @sudwebdesign in #1152
* [IP-1130]: Added required input check on full page loaded to fix #1130 by @AeroBytesNL in #1155
* bug-fix-#1147-error-on-database-migration by @AeroBytesNL in #1159
* Fix setup Red Screen Of Death with bad DB query by @sudwebdesign in #1160
* **[IP-1128]**: Update QrCode.php wrong variable by @VizardAlpha in #1129
* Improve download function in #1127
* Chore: Fix upload_file function in #1141
* Show list of themes on Windows by @RobiNN1 in #1014
* [IP-1038]: Fix for issue 1038: Wrong translation string in setup by @naui95 in #1039
* [IP-1012]: Revert change to invoice_logo() by @jmclaren7 in #1013
* Slight improvements to README.md by @xeruf in #1061
* [IP-1070] by @naui95 in #1071
* [IP-1072] Fixed broken customer-link in projects-widget on dashboard. by @NiklasSchmitt in #1073
* [IP-1078]: ZUGFeRD Name should not be user name by @pumpi in #1079
* [IP-1006]: feature: payments v1.6.2 by @naui95 in #1046
* added information on theming by @naui95 in #1087
* [IP-1097]: add docker publish workflow by @redxtech in #1098
* [IP-841]: Copy Quote UI feedback by @Verony-makesIT in #932
* [IP-998]: copy all fields available in quotes by @naui95 in #1086
* [IP-1083]: Add pagination for tabs in client detail by @pumpi in #1084
* [IP-731]: Add another digit for quantity (new version) by @naui95 in #1112
* [IP-1003]: Add extra field title by nielsdrost7 in #1101
* add function to check post variables by nielsdrost7 in #1090
* [IP-1096] Payments array variable gets lost for invoices by nielsdrost7 in #1104
* Chore: add pint by nielsdrost7 in #1081
* [IP-1033] creation dates of invoices should never be changed by nielsdrost7 in #1040
* [IP-1034]: Fix error where default_language at some point switches to English by nielsdrost7 in #1035
* Make sure we don't get unintended output in our PDF files by nielsdrost7 in #1089
* [IP-1010]: Add php-dom and php-xml to the Dockerfile by nielsdrost7 in #1110
* Special project by nielsdrost7 in #1120
* [IP-1063]: Allowing for Dynamic Properties (**PHP 8.2**) by nielsdrost7 in #1117


Co-authored-by: Thomas Ingles
Co-authored-by: AutiCodes
Co-authored-by: AeroBytes
Co-authored-by: naui95
Co-authored-by: VeRony
Co-authored-by: Marc Heiduk
Co-authored-by: Róbert Kelčák
Co-authored-by: John Mclaren
Co-authored-by: Janek
Co-authored-by: Niklas
Co-authored-by: stephan4p
Co-authored-by: pumpi
Co-authored-by: der-peer
Co-authored-by: Nathan Mattes
Co-authored-by: Kevin Joudrier
Co-authored-by: Gabe Dunn
Co-authored-by: VizardAlpha
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request] copy quote needs confirmation popup model box
3 participants