Conversation
- Purchase api for posting and canceling invoice added - Customer Payment posting and canceling invoice api added - In customer creation api response ( added branch id which automatically gets created for a new customer )
dani7115
changed the title
cambell-prince
left a comment
cambell-prince
left a comment
There was a problem hiding this comment.
Thanks for this Pull Request. It looks pretty good. Summary of the comments:
- It would be nice to see tests for the code added as much as possible.
- There's shouldn't be unnecessary ui code in the api.
| api_error(412, 'Failed to add invoice.'); | ||
| } | ||
| }else{ | ||
| api_error(500, 'Invoice data is invalid.'); |
There was a problem hiding this comment.
User code, like this php, shouldn't return a 5xx response. Use a 4xx response instead. I'd recommend 400 Bad Request. See https://developer.mozilla.org/en-US/docs/Web/HTTP/Status#client_error_responses
| { | ||
| if (!get_post('supplier_id')) | ||
| { | ||
| display_error(_("There is no supplier selected.")); |
There was a problem hiding this comment.
Can you explain why "display_error" is being called from the API which has no ui? I'm not sure why this code is here.
| } catch (Exception $e) { | ||
| error_log($e->getMessage(), 3, "/var/tmp/sales_cancel.log"); | ||
| $resp['msg']='Could not cancel invoice. '; | ||
| return; |
There was a problem hiding this comment.
This fails very quietly. Why not use an api_error
| if (is_closed_trans($_POST['filterType'],$_POST['trans_no'])) | ||
| { | ||
| display_error(_("The selected transaction was closed for edition and cannot be voided.")); | ||
| set_focus('trans_no'); |
There was a problem hiding this comment.
display_error and set_focus looks like ui code from FA. Not sure that this is appropriate for the API
| } | ||
|
|
||
|
|
||
| ?> No newline at end of file |
There was a problem hiding this comment.
Closing tags aren't needed these days. But, does no harm.
| class Purchase_Test extends Crud_Base | ||
| { | ||
|
|
||
| } |
There was a problem hiding this comment.
This test seems to do very little. Can you add a full CRUD test for the purchase cycle. list, count, add, list count+1, read, update, delete, list (count).
| 'trans_no' => 3, | ||
| 'date_' => '10/10/2021', | ||
| 'memo_' => substr('Comment should be limited to 255 characters only. As this is setup in the database field size',0,255), | ||
| ); |
There was a problem hiding this comment.
With your addition of sales_cancel this should be testable. It would be nice to see a test here for this.
| class CustomerPayment_Test extends Crud_Base | ||
| { | ||
|
|
||
| } |
There was a problem hiding this comment.
This test is incomplete. It would be nice to see tests for the code you've added.
2 participants
I have added the following changes to the api module. For extending its further functionality .