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

Allow nested attributes to be accessed #15

Merged
merged 7 commits into from
Nov 8, 2018

Conversation

dsul
Copy link
Contributor

@dsul dsul commented Oct 23, 2018

The current architecture of the Base::attributed method does not allow for access to nested attributes that are a part of the Harvest v2 API. We modified that method in a way that we thought aligned with your vision so that accessors can be dynamically created when nested attributes are passed in.

For example:
The v2 API has id nested within a project object in Time Entries, as opposed to having a top-level project_id accessor like the current architecture assumes. The suggested change would create a project_id accessor by combining the top level project key with each nested attribute.

dsul added 2 commits October 23, 2018 10:20
Copy link
Member

@etagwerker etagwerker left a comment

Choose a reason for hiding this comment

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

@dsul This is very interesting. Thanks for starting the conversation.

I understand that we need a solution for this problem, but I think it would be best to implement something that allowed the user to do this:

time_entries = client.time_entries
time_entry = time_entries.first

expect(time_entry.project.name).to eq(project_name)
expect(time_entry.project.id).to eq(project_id)

In that scenario, we could load time_entry.project with a Harvesting::Models::Project instance and populate it with the right attributes.

What do you think?

@gap777 gap777 force-pushed the allow-nested-attributes branch from d663821 to 570b9f5 Compare October 24, 2018 14:58
@gap777
Copy link
Contributor

gap777 commented Oct 24, 2018

@etagwerker how about now? Your approach was one we considered, but didn't land on initially, but seems easy enough to adopt.

Did you imagine making model classes for things like time_entry.invoice and time_entry.external_reference?

@dsul dsul force-pushed the allow-nested-attributes branch from 570b9f5 to 6c62742 Compare October 24, 2018 15:15
@dsul dsul force-pushed the allow-nested-attributes branch from 6c62742 to 2b7ff2f Compare October 24, 2018 15:25
@gap777
Copy link
Contributor

gap777 commented Oct 30, 2018

@etagwerker @benphelps @mscottford can we get some response to this? Seems like a few PRs have stalled.

Copy link
Member

@etagwerker etagwerker left a comment

Choose a reason for hiding this comment

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

@dsul This is great! One last thing, could you add some examples to README.md?

Also, I'm open to ideas to improve documentation for this new behavior.

@gap777
Copy link
Contributor

gap777 commented Nov 6, 2018

@etagwerker Updates made - what do you think?

Copy link
Member

@etagwerker etagwerker left a comment

Choose a reason for hiding this comment

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

@gap777 @dsul Great, thanks! 💯

@etagwerker etagwerker merged commit 483c44d into fastruby:master Nov 8, 2018
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.

3 participants