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

APIs for invoice list page #208

Merged
merged 23 commits into from
Mar 28, 2022
Merged

APIs for invoice list page #208

merged 23 commits into from
Mar 28, 2022

Conversation

apoorv-mishra
Copy link
Contributor

@apoorv-mishra apoorv-mishra commented Mar 16, 2022

Notion card

https://saeloun.notion.site/API-s-for-Invoice-98d674e7750046b8971590753b8a938a

Summary

Includes API for fetching invoices

Preview

NA

Type of change

  • 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

@apoorv-mishra apoorv-mishra changed the title Invoice api [WIP]API for fetching invoices Mar 16, 2022
@github-actions
Copy link

github-actions bot commented Mar 16, 2022

Current Code Coverage Percent of this PR:

78.98 %

Files having coverage below 100%

Impacted Files Coverage
/app/controllers/concerns/error_handler.rb 73.81 %
/app/controllers/projects_controller.rb 46.15 %
/app/controllers/concerns/timesheet.rb 75.0 %
/app/controllers/invoices_controller.rb 66.67 %
/app/controllers/workspaces_controller.rb 77.78 %
/app/controllers/clients_controller.rb 94.44 %
/app/models/company.rb 90.91 %
/app/models/timesheet_entry.rb 76.92 %
/app/models/project.rb 89.66 %
/app/policies/timesheet_entry_policy.rb 50.0 %
/app/policies/project_policy.rb 85.71 %
/app/policies/client_policy.rb 80.0 %
/app/controllers/users/passwords_controller.rb 41.67 %
/app/controllers/users/omniauth_callbacks_controller.rb 25.0 %
/app/controllers/users/sessions_controller.rb 25.0 %
/app/controllers/users/registrations_controller.rb 40.0 %
/app/controllers/internal_api/v1/projects_controller.rb 75.0 %
/app/controllers/internal_api/v1/reports_controller.rb 88.89 %
/app/controllers/internal_api/v1/timesheet_entry_controller.rb 40.0 %
/app/controllers/internal_api/v1/workspaces_controller.rb 85.71 %
/app/controllers/internal_api/v1/clients_controller.rb 80.77 %
/lib/authentication/google.rb 0.0 %

@pr-triage pr-triage bot added the PR: draft label Mar 16, 2022
@vipulnsward vipulnsward temporarily deployed to miru-review-pr-208 March 16, 2022 16:24 Inactive
@vipulnsward vipulnsward temporarily deployed to miru-review-pr-208 March 17, 2022 07:03 Inactive
@vipulnsward vipulnsward temporarily deployed to miru-review-pr-208 March 23, 2022 05:15 Inactive
@vipulnsward vipulnsward temporarily deployed to miru-review-pr-208 March 23, 2022 13:11 Inactive
@apoorv-mishra apoorv-mishra marked this pull request as ready for review March 23, 2022 13:33
@pr-triage pr-triage bot removed the PR: draft label Mar 23, 2022
@apoorv-mishra apoorv-mishra changed the title [WIP]API for fetching invoices APIs for invoice list page Mar 23, 2022
@supriya3105
Copy link
Contributor

@shivamsinghchahar can you review this PR

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.

Few suggestions, rest looks good.

app/controllers/internal_api/v1/invoices_controller.rb Outdated Show resolved Hide resolved
app/models/company.rb Outdated Show resolved Hide resolved
app/models/invoice.rb Show resolved Hide resolved
config/initializers/pagy.rb Show resolved Hide resolved
app/controllers/internal_api/v1/invoices_controller.rb Outdated Show resolved Hide resolved
spec/models/invoice_spec.rb Outdated Show resolved Hide resolved
Copy link
Contributor

@shivamsinghchahar shivamsinghchahar left a comment

Choose a reason for hiding this comment

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

@apoorv-mishra Please look at the comments below

app/controllers/internal_api/v1/invoices_controller.rb Outdated Show resolved Hide resolved
app/controllers/internal_api/v1/invoices_controller.rb Outdated Show resolved Hide resolved
app/controllers/internal_api/v1/invoices_controller.rb Outdated Show resolved Hide resolved
app/controllers/internal_api/v1/invoices_controller.rb Outdated Show resolved Hide resolved
app/controllers/internal_api/v1/invoices_controller.rb Outdated Show resolved Hide resolved
app/controllers/internal_api/v1/invoices_controller.rb Outdated Show resolved Hide resolved
app/models/invoice.rb Show resolved Hide resolved
config/initializers/pagy.rb Show resolved Hide resolved
spec/models/invoice_spec.rb Outdated Show resolved Hide resolved
spec/models/invoice_spec.rb Outdated Show resolved Hide resolved
spec/models/invoice_spec.rb Outdated Show resolved Hide resolved
spec/requests/internal_api/v1/invoices/index_spec.rb Outdated Show resolved Hide resolved
spec/requests/internal_api/v1/invoices/index_spec.rb Outdated Show resolved Hide resolved
Copy link
Contributor

@shivamsinghchahar shivamsinghchahar left a comment

Choose a reason for hiding this comment

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

@apoorv-mishra Please look at the comments below

app/controllers/internal_api/v1/invoices_controller.rb Outdated Show resolved Hide resolved
spec/requests/internal_api/v1/invoices/index_spec.rb Outdated Show resolved Hide resolved
spec/requests/internal_api/v1/invoices/index_spec.rb Outdated Show resolved Hide resolved
app/views/internal_api/v1/invoices/index.json.jbuilder Outdated Show resolved Hide resolved
@vipulnsward vipulnsward temporarily deployed to miru-review-pr-208 March 25, 2022 14:52 Inactive
Copy link
Contributor

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

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

app/models/invoice.rb Show resolved Hide resolved
app/controllers/concerns/error_handler.rb Show resolved Hide resolved
spec/rails_helper.rb Outdated Show resolved Hide resolved
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

@vipulnsward vipulnsward temporarily deployed to miru-review-pr-208 March 28, 2022 13:41 Inactive
@vipulnsward vipulnsward temporarily deployed to miru-review-pr-208 March 28, 2022 13:49 Inactive
@apoorv-mishra apoorv-mishra merged commit 885d7c0 into develop Mar 28, 2022
@@ -41,7 +41,7 @@

describe "Associations" do
describe "belongs to" do
it { is_expected.to belong_to(:company) }
it { is_expected.not_to belong_to(:company) }
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can just remove this spec? There is no point of checking for absent belongs_to associations.

Copy link
Contributor

Choose a reason for hiding this comment

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

On second thought, why isn't there a company association to an invoice or am I missing something?

end
end

describe ".delegate" do
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
describe ".delegate" do
describe "Delegates" do

@akhilgkrishnan akhilgkrishnan deleted the invoice-api branch April 5, 2022 04:59
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.

8 participants