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

Feature/add method handling expansion child objects #1063

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

alan-ms
Copy link
Contributor

@alan-ms alan-ms commented Mar 24, 2022

Description:

It addresses the problem when we want to expand child objects in jbuilder.

Closes #962
Co-authored-by: MaiconMares maiconmaresunb@gmail.com

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.

  "id": "1",
  "object": "transaction",
  "created": 1648087371,
  "supporter": {
    "id": 1,
    "name": null,
    "organization": null,
    "phone": null,
    "anonymous": false,
    "deleted": false,
    "object": "supporter",
    "merged_into": null,
    "supporter_addresses": [
      1
    ],
    "url": "http://localhost:3000/api/nonprofits/1/supporters/1",
    "nonprofit": 1
  },
  "nonprofit": 1,
  "amount": {
    "cents": 10,
    "currency": "usd"
  },
  "subtransaction": {
    "type": "subtransaction",
    "supporter": 1,
    "nonprofit": 1,
    "transaction": "1",
    "payments": [
      {
        "type": "payment",
        "id": "1",
        "supporter": 1,
        "nonprofit": 1,
        "transaction": "1",
        "subtransaction": {
          "id": "1",
          "object": "offline_transaction",
          "type": "subtransaction"
        },
        "object": "offline_transaction_charge",
        "net_amount": {
          "cents": 100,
          "currency": "usd"
        },
        "gross_amount": {
          "cents": 100,
          "currency": "usd"
        },
        "fee_total": {
          "cents": 100,
          "currency": "usd"
        },
        "created": 0,
        "url": "http://localhost:3000/api/nonprofits/1/transactions/1/subtransaction/payments/1"
      }
    ],
    "url": "http://localhost:3000/api/nonprofits/1/transactions/1/subtransaction",
    "object": "offline_transaction",
    "created": 1648087371,
    "amount": {
      "cents": 100,
      "currency": "usd"
    },
    "net_amount": {
      "cents": 100,
      "currency": "usd"
    }
  },
  "transaction_assignments": [
    {
      "type": "trx_assignment",
      "id": "1",
      "supporter": 1,
      "nonprofit": 1,
      "transaction": "1",
      "object": "donation",
      "designation": null,
      "amount": {
        "cents": 100,
        "currency": "usd"
      }
    }
  ],
  "url": "http://localhost:3000/api/nonprofits/1/transactions/1"
}```

alan-ms and others added 4 commits March 24, 2022 09:36
Co-authored-by: MaiconMares <maiconmaresunb@gmail.com>
Co-authored-by: MaiconMares <maiconmaresunb@gmail.com>
Co-authored-by: MaiconMares <maiconmaresunb@gmail.com>
Co-authored-by: MaiconMares <maiconmaresunb@gmail.com>
@MaiconMares
Copy link
Contributor

@wwahammy can you review our Pull Request and give feedbacks or guidances on what we need to improve/fix?

Copy link
Member

@wwahammy wwahammy left a 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:

  1. Add support for handling the expansion of child objects and
  2. 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!

Comment on lines 1 to 18
# 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
Copy link
Member

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.

Copy link
Contributor

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
Copy link
Member

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.

Copy link
Contributor

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
Copy link
Member

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.

Copy link
Contributor

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
Copy link
Member

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.

Copy link
Contributor

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
Copy link
Member

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.

Copy link
Contributor

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
Copy link
Member

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.

Copy link
Contributor

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
Copy link
Member

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.

Copy link
Contributor

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)
Copy link
Member

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 <alan.sousa@protonmail.com>
@MaiconMares MaiconMares force-pushed the feature/add-method-handling-expansion-child-objects branch from 2e2960e to 8961fdb Compare April 23, 2022 21:25
alan-ms and others added 2 commits April 23, 2022 18:32
Co-authored-by: MaiconMares <maiconmaresunb@gmail.com>
Co-authored-by: Alan Marques <alan.sousa@protonmail.com>
@MaiconMares MaiconMares force-pushed the feature/add-method-handling-expansion-child-objects branch from 2635680 to 7dc0007 Compare April 24, 2022 21:19
Co-authored-by: Alan Marques <alan.sousa@protonmail.com>
@MaiconMares
Copy link
Contributor

MaiconMares commented Apr 24, 2022

Hi @wwahammy, I and @alan-ms have removed the unrelated files and refine the solution, we think the problem is solved.
Obs.: however the test from CommitChange doesn't work with our solution. We've thought it should work for our solution due to your recommendation, but it doesn't fit at all.

@wwahammy
Copy link
Member

Thanks @MaiconMares! When you say it "doesn't fit at all", what do you mean?

@MaiconMares
Copy link
Contributor

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.

@wwahammy wwahammy marked this pull request as draft July 18, 2022 21:01
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.

[FEATURE] Add a method for handling expansion of child objects in jbuilder
3 participants