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

Added new apis #56

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

dani7115
Copy link

@dani7115 dani7115 commented Nov 8, 2021

I have added the following changes to the api module. For extending its further functionality .

  • Sales Api Cancel invoice method update ( working now )
  • 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 )

- 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 dani7115 changed the title - Sales Api Cancel invoice method update ( working now ) Added new apis Nov 8, 2021
Copy link
Collaborator

@cambell-prince cambell-prince left a comment

Choose a reason for hiding this comment

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

Thanks for this Pull Request. It looks pretty good. Summary of the comments:

  1. It would be nice to see tests for the code added as much as possible.
  2. 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.');
Copy link
Collaborator

Choose a reason for hiding this comment

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

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."));
Copy link
Collaborator

Choose a reason for hiding this comment

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

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;
Copy link
Collaborator

Choose a reason for hiding this comment

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

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');
Copy link
Collaborator

Choose a reason for hiding this comment

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

display_error and set_focus looks like ui code from FA. Not sure that this is appropriate for the API

}


?>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Closing tags aren't needed these days. But, does no harm.

class Purchase_Test extends Crud_Base
{

}
Copy link
Collaborator

Choose a reason for hiding this comment

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

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),
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

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
{

}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This test is incomplete. It would be nice to see tests for the code you've added.

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.

2 participants