-
Notifications
You must be signed in to change notification settings - Fork 94
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
Feature/add method handling expansion child objects #1063
base: main
Are you sure you want to change the base?
Feature/add method handling expansion child objects #1063
Conversation
Co-authored-by: MaiconMares <[email protected]>
Co-authored-by: MaiconMares <[email protected]>
Co-authored-by: MaiconMares <[email protected]>
Co-authored-by: MaiconMares <[email protected]>
@wwahammy can you review our Pull Request and give feedbacks or guidances on what we need to improve/fix? |
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.
Thanks for the work.
I think it'd be helpful to step back for a second and consider what the problem you're trying to solve is. How do all of the changes you've made help you do the following:
- Add support for handling the expansion of child objects and
- Prove that support works as expected.
A lot of these files here don't actually assist in that effort and therefore, they shouldn't be included. I believe there is testing around simple objects and that expansions work correctly in the source tree in CommitChange. I would recommend starting with that for your testing.
To be clear, you don't need to even use the new expansion code for all of the endpoints in this PR. In fact, it'd probably be better if you didn't!
# frozen_string_literal: true | ||
|
||
# License: AGPL-3.0-or-later WITH WTO-AP-3.0-or-later | ||
# Full license explanation at https://github.com/houdiniproject/houdini/blob/main/LICENSE | ||
|
||
json.object 'stripe_transaction_dispute_reversal' | ||
|
||
json.net_amount do | ||
json.partial! '/api_new/common/amount', amount: paymentable.net_amount_as_money | ||
end | ||
|
||
json.gross_amount do | ||
json.partial! '/api_new/common/amount', amount: paymentable.gross_amount_as_money | ||
end | ||
|
||
json.fee_total do | ||
json.partial! '/api_new/common/amount', amount: paymentable.fee_total_as_money | ||
end |
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.
I don't believe this file has anything to do with the specific change.
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.
Done!
@@ -0,0 +1,12 @@ | |||
# frozen_string_literal: true |
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.
I don't believe this file has anything to do with the specific change.
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.
Done!
@@ -0,0 +1,18 @@ | |||
# frozen_string_literal: true |
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.
I don't believe this file has anything to do with the specific change.
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.
Done!
@@ -0,0 +1,12 @@ | |||
# frozen_string_literal: true |
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.
I don't believe this file has anything to do with the specific change.
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.
Done!
@@ -0,0 +1,18 @@ | |||
# frozen_string_literal: true |
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.
I don't believe this file has anything to do with the specific change.
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.
Done!
@@ -0,0 +1,12 @@ | |||
# frozen_string_literal: true |
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.
I don't believe this file has anything to do with the specific change.
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.
Done!
@@ -0,0 +1,16 @@ | |||
# frozen_string_literal: true |
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.
I don't believe this file has anything to do with the specific change.
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.
Done!
@@ -2,17 +2,17 @@ | |||
|
|||
# License: AGPL-3.0-or-later WITH WTO-AP-3.0-or-later | |||
# Full license explanation at https://github.com/houdiniproject/houdini/blob/main/LICENSE | |||
json.(subtransaction.subtransactable, :id) |
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.
I think this should be kept for now.
Co-authored-by: Alan Marques <[email protected]>
2e2960e
to
8961fdb
Compare
Co-authored-by: MaiconMares <[email protected]>
Co-authored-by: Alan Marques <[email protected]>
2635680
to
7dc0007
Compare
Co-authored-by: Alan Marques <[email protected]>
Thanks @MaiconMares! When you say it "doesn't fit at all", what do you mean? |
@wwahammy I mean it doesn't work for any of our cases. |
Description:
It addresses the problem when we want to expand child objects in jbuilder.
Closes #962
Co-authored-by: MaiconMares [email protected]
Motivation for implementing it:
At the moment we don't have a way to expand child objects in jbuilder, so we implemented an approach where this is possible.
Solution summary:
Modified json partial d transaction, adding expansion of child objects.