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

[BE]APIs to save invoices #256

Merged
merged 6 commits into from
Apr 14, 2022
Merged

[BE]APIs to save invoices #256

merged 6 commits into from
Apr 14, 2022

Conversation

apoorv-mishra
Copy link
Contributor

Notion card

https://www.notion.so/saeloun/6afc9500b2cc42af86be55f37894fb09?v=b4528f26f4424af7beaf036262796cb3&p=28db3bab184142269bb66230afa67635

Summary

This contains APIs to create and update invoices.

Preview

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Checklist:

  • I have manually tested all workflows
  • I have performed a self-review of my own code
  • I have added automated tests for my code

@vipulnsward vipulnsward temporarily deployed to miru-review-pr-256 April 13, 2022 20:03 Inactive
@apoorv-mishra apoorv-mishra temporarily deployed to miru-review-pr-256 April 13, 2022 20:14 Inactive
@github-actions
Copy link

github-actions bot commented Apr 13, 2022

Current Code Coverage Percent of this PR:

91.28 %

Files having coverage below 100%

Impacted Files Coverage
/app/controllers/clients_controller.rb 94.44 %
/app/controllers/invoices_controller.rb 66.67 %
/app/controllers/concerns/timesheet.rb 75.0 %
/app/models/project.rb 90.32 %
/app/controllers/users/registrations_controller.rb 40.0 %
/app/controllers/users/sessions_controller.rb 25.0 %
/app/controllers/users/omniauth_callbacks_controller.rb 25.0 %
/app/controllers/users/passwords_controller.rb 41.67 %
/app/controllers/internal_api/v1/timesheet_entry_controller.rb 40.0 %
/app/controllers/internal_api/v1/projects_controller.rb 75.0 %
/app/controllers/internal_api/v1/timesheet_entry/bulk_action_controller.rb 50.0 %


def update
authorize invoice
if invoice.update!(invoice_params)
Copy link
Member

@akhilgkrishnan akhilgkrishnan Apr 14, 2022

Choose a reason for hiding this comment

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

never use bang with update for an if condition. if something fails it will raise an error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's fine since we're handling the errors in error_handler.rb. It'll show up in the API response.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For example, if you send

{
    "reference": "foo",
    "issue_date": "2022-02-01",
    "due_date": "2022-01-31"
}

you get

{
    "errors": {
        "due_date": [
            "must be greater than or equal to 2022-02-01"
        ]
    },
    "notice": "Failed to saved changes"
}

Which is fine, right?

Copy link
Member

@akhilgkrishnan akhilgkrishnan Apr 14, 2022

Choose a reason for hiding this comment

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

Yes, it will rescue the error, but it wont return any boolean. So you can remove if condition, because there is no use of if condition here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ohh, yes if is unnecessary. Thanks for pointing out. Fixed.

@apoorv-mishra apoorv-mishra temporarily deployed to miru-review-pr-256 April 14, 2022 04:58 Inactive
Copy link
Contributor

@keshavbiswa keshavbiswa left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@rohitjoshixyz rohitjoshixyz left a comment

Choose a reason for hiding this comment

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

One suggestion

Comment on lines 3 to 19
json.key_format! camelize: :lower
json.deep_format_keys!
json.id invoice.id
json.invoice_number invoice.invoice_number
json.issue_date invoice.issue_date
json.due_date invoice.due_date
json.reference invoice.reference
json.amount invoice.amount
json.outstanding_amount invoice.outstanding_amount
json.amount_paid invoice.amount_paid
json.amount_due invoice.amount_due
json.discount invoice.discount
json.tax invoice.tax
json.status invoice.status
json.client do
json.name client.name
end
Copy link
Contributor

Choose a reason for hiding this comment

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

These attributes are repeating in both create and update JSON builder, we can extract them into a _record.json.builder partial and use json.partial! to reuse it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the tip! Fixed.

@apoorv-mishra apoorv-mishra temporarily deployed to miru-review-pr-256 April 14, 2022 06:46 Inactive
authorize Invoice
render :create, locals: {
invoice: Invoice.create!(invoice_params),
client: Client.find(invoice_params[:client_id])
Copy link
Contributor

Choose a reason for hiding this comment

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

Move Client.find into a different method load_client and use that instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Comment on lines 22 to 28
invoice: {
client_id: company.clients.first.id,
invoice_number: "INV0001",
reference: "bar",
issue_date: "2022-01-01",
due_date: "2022-01-31"
})
Copy link
Contributor

@shivamsinghchahar shivamsinghchahar Apr 14, 2022

Choose a reason for hiding this comment

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

Suggested change
invoice: {
client_id: company.clients.first.id,
invoice_number: "INV0001",
reference: "bar",
issue_date: "2022-01-01",
due_date: "2022-01-31"
})
invoice: attributes_for :invoice, client: company.clients.first

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@apoorv-mishra apoorv-mishra temporarily deployed to miru-review-pr-256 April 14, 2022 08:31 Inactive
Copy link
Member

@akhilgkrishnan akhilgkrishnan left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@rohitjoshixyz rohitjoshixyz left a comment

Choose a reason for hiding this comment

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

LGTM

@apoorv-mishra apoorv-mishra merged commit 6b8638a into develop Apr 14, 2022
@shivamsinghchahar shivamsinghchahar deleted the create-invoice branch April 14, 2022 09:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants