Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 transactions resource #92
Create transactions resource #92
Changes from 58 commits
d86d3c4
bbadaff
f01163b
99105ed
4cd9bf3
ed96735
97bd9ac
9a21a28
ee66825
c2fd00e
d4482ad
19641b6
441161e
4f79002
2fc3690
41f020e
42acf1d
c628594
09b06a3
2435700
6ab802a
9d9b551
54d42c4
8835a93
0d2e15e
e902ace
00f977f
199059d
b2de999
9ded60e
83750f8
764645c
3b21155
96cc2f0
8aad7e4
edc2d1a
1105b26
ccb6ac3
a48879f
2ec09bc
b971e75
2d23ab0
a537868
f179828
2d7ce19
8de7f2b
355f91f
2613bff
165390f
84af5df
cd14334
83b9fd0
a87d72c
20224e2
f6dc030
26036ad
826dddf
3970102
68d1d25
fe39084
4e049f7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
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.
Fix:
Should be:
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.
hmmm, are you sure?
would nest know to add a
.donater
attr or function to the transation model then? now that i think about it, idk how to control the model in nest, like if i wanted to write a custom function likeasset.close()
(that updates the status to all the transactions related to that asset, how would i do that?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.
Looking at your example,
This:
can be replaced by example below, if you change the createDto to use poster_id instead of a whole new poster object. The return type will be whatever the repository returns (
transactionEntity
):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.
Then, you would have to implement something like: create-transactions-resource...sample/create-transactions-resource
Also, if you are going to stick with the
getTransactionsDto
as the get all filtering model, there is some funky formatting we have to do in the front end to send the nested object in the url query params. See thisThere 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.
Fix:
Should be:
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.
Same as above, only the foreign key is required
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.
Same as above, only the foreign key is required
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.
Don't these cancel each other?
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.
totally 😆
removed altogether because the default value should be used and so we don't need to pass anything in
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 should either be
updateTransactionStatus
or be able to update any propertyi'm leaning towards the latter unless you see a benefit in having multiple endpoints for updating different properties