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

Create invoice page #64

Merged
merged 16 commits into from
Feb 9, 2025
Merged

Create invoice page #64

merged 16 commits into from
Feb 9, 2025

Conversation

lanaramadan
Copy link
Contributor

@lanaramadan lanaramadan commented Feb 6, 2025

Description

Created InvoiceTitle, InvoiceStats (which includes payees), InvoicePayments components for the single Invoice page. Also added pagination.

Screenshots/Media

image image

Issues

Closes #53

@lanaramadan lanaramadan linked an issue Feb 6, 2025 that may be closed by this pull request
9 tasks
@lanaramadan lanaramadan force-pushed the 53-create-invoice-page branch from 949914d to f725ddf Compare February 6, 2025 08:52
@lanaramadan
Copy link
Contributor Author

lanaramadan commented Feb 6, 2025

One thing we weren't confident with is the "Remaining Balance" section.

Since it depends on the total and paid values of each invoice we had 3 options:

  1. Somehow call invoices/total/:id and invoices/paid/:id inside /events/remainder/:id — this only seemed feasible if we used axios.get, which we weren’t sure what endpoint to use then (also, apparently this isn’t really recommended online for some reason) and the endpoints weren't actually fully implemented (nor did we know how to implement them since it's unclear where those values would be derived from).
  2. Reusing the code in invoices/total/:id and /invoices/paid/:id in a third endpoint. But the endpoints aren’t fully implemented and it would've led to bad modularity.
  3. Use JS to calculate the remaining balance.

We went with the third option, but let us know if we should change anything (and how).

@lanaramadan lanaramadan force-pushed the 53-create-invoice-page branch from f725ddf to 224ad62 Compare February 6, 2025 08:56
@lanaramadan lanaramadan changed the title 53 create invoice page Create invoice page Feb 6, 2025
Copy link
Collaborator

@theNatePi theNatePi left a comment

Choose a reason for hiding this comment

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

Good work guys! Some edits:

  • Some invoices show no data, such as invoice with ID = 4
  • The "Program:" title at the top seems to be showing the description of the program, when it should show the name of the program
  • The "Billed To" Section should only show the payees, not the instructor
  • I think you left some console.log's in there, make sure to remove them
  • In the invoice page, you call await backend.get("invoices/" + id); twice. Make sure to only do this once and to re-use the first content
  • It seems that the indent spacing in events.js (the router) went from 2 spaces to 4. Make sure all tab spacing is 2 spaces
  • The invoices router might also have some new indent spacing issues, make sure it is also consistant

Don't worry about changing the remaining balance implementation you have

Important

Make sure to pull before making modifications
Reminder: These fixes are due Tmr at midnight

Very good work on the table, it works really well!
@lanaramadan @AstonKiChan

- switched description to title
- payees not instructor
- removed console logs
- re-use routes
- fixed indents
Was because all the fields would be set to null if smth was missing (ie. payees)
@lanaramadan lanaramadan requested a review from theNatePi February 7, 2025 05:06
@lanaramadan lanaramadan force-pushed the 53-create-invoice-page branch from f61697f to 8174eff Compare February 7, 2025 05:12
@lanaramadan
Copy link
Contributor Author

Nonexistent invoices:
image

Invoices with missing fields:
image

Invoices with no issues:
image

Was returning an error if there was no payees
@brelieu05
Copy link
Contributor

my bad, accidentally changed the label thinking it was my PR

@theNatePi
Copy link
Collaborator

LGTM!

jessieh9
jessieh9 previously approved these changes Feb 9, 2025
@jessieh9 jessieh9 merged commit 4aff458 into main Feb 9, 2025
1 check passed
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.

Create Invoice Page
5 participants