Skip to content
This repository was archived by the owner on Apr 3, 2024. It is now read-only.

Fix Read API calls#9

Closed
bezoar17 wants to merge 3 commits into
privateprep:fix-read-callsfrom
bezoar17:master
Closed

Fix Read API calls#9
bezoar17 wants to merge 3 commits into
privateprep:fix-read-callsfrom
bezoar17:master

Conversation

@bezoar17
Copy link
Copy Markdown
Contributor

Fix read types of Intacct API calls e.g. https://developer.intacct.com/api/accounts-payable/vendors/#get-vendor
Ref: #7

@danielrose7
Copy link
Copy Markdown
Contributor

Thanks for the PR. Always appreciate the help!

I am thinking that rather than add a few levels of complexity to try to get the tag's case to match the API specification, it would make more sense to remove the case transformation logic. The gem would then instead rely on the case of the key that is passed

What do you think?

@bezoar17
Copy link
Copy Markdown
Contributor Author

bezoar17 commented Feb 13, 2018

Hey, makes sense to rely on the provided keys. I have updated the code, ReadMe and tests accordingly. Only object_type is forcefully upcased. Let me know if we want object_type also to be taken as is.

Copy link
Copy Markdown
Contributor

@toddsiegel toddsiegel left a comment

Choose a reason for hiding this comment

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

This looks good. Thanks, @bezoar17. IMO, I think the code commits should be squashed down to one or two cohesive commits, with descriptive commit messages. The README commit can be left as-is. What do you think @danielpowell4?

Comment thread lib/intacct_ruby/version.rb Outdated
@@ -1,3 +1,3 @@
module IntacctRuby
VERSION = '1.7.0'.freeze
VERSION = '1.8.0'.freeze
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No need to bump the version, we'll do that when we cut a new version of the gem.

Comment thread README.md
</operation>
</request>
```

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the doc updates!

@danielrose7
Copy link
Copy Markdown
Contributor

danielrose7 commented Feb 13, 2018

@bezoar17, I agree with @toddsiegel. If you could squash down your commits, we would appreciate it.

What we mean by this is that these commits make a change and only to then undo it a step later. We prefer one-directional code stories when possible.

As you will notice when you click through the git blame of any of the commits in this gem, they tell a specific story and often include examples of what is going on.

All of this work (sans the ruby and gem version bumps) could be broken into a single commit along the lines of:

Allow for varying cases in request keys

The Intacct API calls for different cases in read and write requests. For example, a read
would look like:
[insert basic snippet]

A write, on the other hand looks like:
[insert basic snippet]

Before this commit, the gem was artificially upcasing all keys it was passed as hashes.
This meant that it did not work with any of the read queries. This commit removes
this iterative upcase instead keying on the case of the key which it is passed.

This is a breaking change.

@bezoar17 Did you by change run the test suite and assess the damage?
If there are several failures, a second commit could address these.

@bezoar17
Copy link
Copy Markdown
Contributor Author

Updated with cleaner commits. @toddsiegel @danielpowell4 .
I did run the test suite and there are no failures :)

@danielrose7
Copy link
Copy Markdown
Contributor

@bezoar17, again, thanks for the work!

What do you think about removing the :upcase of CU calls altogether?

Sorry for the slow reply. We are working on rearranging some permissions on this repo and making a roadmap for the future.

The Intacct API calls for different format and cases in read and write requests.

For example, a read request would look like:

  request.readByQuery(nil, { object: 'VENDOR',
                             query: '',
                             fields: '*',
                             pagesize: 100})

Note: Object-Type VENDOR is passed as a value in key-value pair and the keys are lowercase.

A write request, on the other hand looks like:

  request.create(:CUSTOMER, { CUSTOMERID: '1',
                              FIRST_NAME: 'Han',
                              LAST_NAME: 'Solo',
                              TYPE: 'Person',
                              EMAIL1: 'han@solo.com',
                              STATUS: 'active'
                            })

Note: Object-Type CUSTOMER is passed as the first argument and is in uppercase along with the keys.

Before this commit, the gem was artificially upcasing all keys it was passed as hashes.
This meant that it did not work with any of the read queries. This commit removes
this iterative upcase instead keying on the case of the key which it is passed.It also adds
a new function type readMore to allowed function types.

This is a breaking change.
@bezoar17
Copy link
Copy Markdown
Contributor Author

@danielpowell4 , removed forceful upcase for CU too.

@danielrose7
Copy link
Copy Markdown
Contributor

@bezoar17 I have seen this. Looks good.

Getting travisCI hooked up as I type. Hope to have this out in a new release this/next week!

@danielrose7 danielrose7 mentioned this pull request Mar 8, 2018
1 task
@danielrose7 danielrose7 changed the base branch from master to fix-read-api-calls March 8, 2018 19:14
@danielrose7 danielrose7 closed this Mar 8, 2018
@danielrose7 danielrose7 changed the base branch from fix-read-api-calls to fix-read-calls March 8, 2018 19:28
@danielrose7 danielrose7 reopened this Mar 8, 2018
@danielrose7 danielrose7 mentioned this pull request Mar 8, 2018
3 tasks
@danielrose7
Copy link
Copy Markdown
Contributor

@bezoar17 Thanks again!

I cherry picked your commits and merge in another branch as these conflicts arose.

Aiming to cut a new version of the gem (1.8) today

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants