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

throw RestException(500) if update of invoice fails #32134 #32190

Merged
merged 4 commits into from
Dec 5, 2024

Conversation

JonBendtsen
Copy link
Contributor

FIX #32134 Updating Invoice Notes
throw RestException(500) if update of invoice fails

@JonBendtsen
Copy link
Contributor Author

Even though @natalieboucher said it worked in version 19, I decided to make invoice API use same structure as order API, and from v18 because v18 are still released as LTS.

@JonBendtsen JonBendtsen marked this pull request as ready for review December 1, 2024 09:27
@JonBendtsen
Copy link
Contributor Author

devcamp munich 2024

@frederic34
Copy link
Member

@JonBendtsen it will throw a phan error in develop like in your closed PR
https://github.com/Dolibarr/dolibarr/pull/32189/files

@JonBendtsen
Copy link
Contributor Author

it will throw a phan error in develop like in your closed PR

Are orders any different than invoices?

commande/class/api_orders.class.php

		if ($this->commande->update(DolibarrApiAccess::$user) > 0) {
			return $this->get($id);
		} else {
			throw new RestException(500, $this->commande->error);
		}

compta/facture/class/api_invoices.class.php

		if ($this->invoice->update(DolibarrApiAccess::$user) > 0) {
			return $this->get($id);
		} else {
			throw new RestException(500, $this->invoice->error);
		}

Because all I am doing is making the code pretty identical

The public function update( seems pretty similar between orders
https://github.com/Dolibarr/dolibarr/blob/develop/htdocs/compta/facture/class/facture.class.php#L2500

and invoices
https://github.com/Dolibarr/dolibarr/blob/develop/htdocs/commande/class/commande.class.php#L3349

Am I wrong in expecting that phan would also throw an error if the change was in the existing order code?

I also actually tested this code, and with it I do get an error in the API, where as without it I get http code 200 as if no errors

@eldy eldy added the PR to fix - See feedback in comments PR needs to be fixed to be integrated (some comments should describes the fix to do) label Dec 1, 2024
Copy link
Contributor

@rycks rycks left a comment

Choose a reason for hiding this comment

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

ok for me thanks !

@eldy eldy merged commit a26eecf into Dolibarr:18.0 Dec 5, 2024
6 checks passed
@JonBendtsen JonBendtsen deleted the v18_Updating_Invoice_Notes_32134 branch December 12, 2024 21:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Event: DevCamp 2024 Munich PR to fix - See feedback in comments PR needs to be fixed to be integrated (some comments should describes the fix to do)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants