-
Notifications
You must be signed in to change notification settings - Fork 1
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: allow listing all invoices #10
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good!
Only a couple of small things.
It'd also be nice if you could write tests for the feature. It should easy to write a couple of tests based on the existing ones.
src/database/invoices.rs
Outdated
.select((Invoice::as_select(), Party::as_select())) | ||
.load::<(Invoice, Party)>(&mut self.0) | ||
.await? | ||
.into_iter() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
diesel_async supports streams that can be directly collected into a container that is not Vec<T>
See: https://docs.rs/diesel-async/latest/diesel_async/trait.RunQueryDsl.html#method.load_stream
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed this in d6d41c0 to use the recommended method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added two new tests, not entirely sure how testing should be approached in a project like this (or in general :D) so feedback would be appreciated
src/database/invoices.rs
Outdated
.select((Invoice::as_select(), Party::as_select())) | ||
.load::<(Invoice, Party)>(&mut self.0) | ||
.await? | ||
.into_iter() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed this in d6d41c0 to use the recommended method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
Thanks for your contribution!
Allows listing all populated invoices with
GET /invoices