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

Feat/BILL-CPC-1046) Allows a User to get all their bills (db bug fix included) #629

Conversation

Futurespast
Copy link
Collaborator

JIRA: https://champlainsaintlambert.atlassian.net/browse/CPC-1046?atlOrigin=eyJpIjoiYmE3MTgzYzlhZWQ4NGQ4YmI4ZTRhMWYwYjgwYTg2YjgiLCJwIjoiaiJ9

Context:

This ticket creates the main billing page for the customer and allows them to see all their bills (by customer id). This way the billing in the nav bar now redirects them to their billing page and the customer can see all their bills. Please note, I have accidently fixed and properly configured the database in this branch. Therefore a new branch will not be created despite the separate story.

Changes

  • creation of a new customer billing page
  • a billing table to get and handle all the bills
  • bill model in the front end
  • modified path.routes, router and AppNavBar to have the frontend access the newly created page
  • fixed the mongo db to actually have customer ids and moved it to the proper layer
  • added new controller for old v2 angular frontend

Added to the UI (No changes to original UI)

image

Copy link
Collaborator

@flyindonut flyindonut left a comment

Choose a reason for hiding this comment

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

Why not use the @SecuredEndpoint annotation for your endpoint in the v2 controller? I think you should write tests to cover the implementation of your endpoint. Also in BillsListTable.tsx, you're using the endpoint of the old API gateway as opposed to the new one you created. You are also using fetch instead of the axiosInstance in the shared folder.

The rest looks good to me!

@Futurespast
Copy link
Collaborator Author

The tests for the endpoint where already written, they still pass with the new changes. I didn't use secured annotation because the original billing endpoint in controller didn't use it so I thought it would be ok. I have to use fetch because the axiosinstance was being super weird in processing the flux. I had to try a bunch of different ways to get it to work.

@flyindonut
Copy link
Collaborator

flyindonut commented Sep 14, 2024

The tests for the endpoint where already written, they still pass with the new changes. I didn't use secured annotation because the original billing endpoint in controller didn't use it so I thought it would be ok. I have to use fetch because the axiosinstance was being super weird in processing the flux. I had to try a bunch of different ways to get it to work.

Since you created a new v2 endpoint, I think it would be good to test your new implementation. Also I noticed that the URL contains the customer id. Wouldn't it be better to make the route be '/customer/bills'? Since you are using the logged in customer's id to perform the request you don't really need to show it in the URL. Each customer will see their corresponding bills when accessing the page.

@Futurespast
Copy link
Collaborator Author

The tests for the endpoint where already written, they still pass with the new changes. I didn't use secured annotation because the original billing endpoint in controller didn't use it so I thought it would be ok. I have to use fetch because the axiosinstance was being super weird in processing the flux. I had to try a bunch of different ways to get it to work.

Since you created a new v2 endpoint, I think it would be good to test your new implementation. Also I noticed that the URL contains the customer id. Wouldn't it be better to make the route be '/customer/bills'? Since you are using the logged in customer's id to perform the request you don't really need to show it in the URL. Each customer will see their corresponding bills when accessing the page.

Ok thanks. I will makes these changes. Do I close the pull request?

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