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

Fix the format of xml for read requests#16

Merged
danielrose7 merged 1 commit into
privateprep:masterfrom
bezoar17:issue_15
Mar 19, 2018
Merged

Fix the format of xml for read requests#16
danielrose7 merged 1 commit into
privateprep:masterfrom
bezoar17:issue_15

Conversation

@bezoar17
Copy link
Copy Markdown
Contributor

@bezoar17 bezoar17 commented Mar 19, 2018

Contributor Self-Check:

  • My commits and messages are solid
  • I have included tests that check success and failure
  • I have updated the README as appropriate

Which GitHub Issues does this PR address?

#15

What does this PR do?

Fix the format of XML query for read API calls.
When, the PR #9 was ported to PR #13, a few lines were missed due to which this issue persisted. This should fix that too.

How do I manually test this?

Steps to reproduce are mentioned in the issue itself.

Additional Comments

No need to update README.md as those were updated with PR#9 itself.

GIF for how this PR makes me feel

For read requests like this
  gateway.read(nil, {object: "VENDOR", pagesize: 100, fields: '*'})

there was an extra tag being thrown

<read>
    <>
    <object>VENDOR</object>
    <keys>1</keys>
    <fields>*</fields>
    <>
</read>

as there was no distinction between the Create,Update and the Read requests while building the xml query. This check has been restored
@bezoar17
Copy link
Copy Markdown
Contributor Author

@danielpowell4 Hey, while porting PR 9 to 13, you missed some lines. Restored them.

@danielrose7
Copy link
Copy Markdown
Contributor

danielrose7 commented Mar 19, 2018

@bezoar17 Sorry about this oversight on my part. I saw these changes but I didn't realize the need when I ran it against our integration as we only are using the gem for CU_TYPES currently

I think this is fine for a quick patch that I can put out quickly


It seems to me that since the gem now only supports ruby 2+ there is likely a way to clean up the gem's API with keyword arguments such that passing in "nil" as the second argument for read calls becomes unnecessary

To me, this looks better:

request = IntacctRuby::Request.new(REQUEST_OPTS)

request.readByQuery parameters: {
  object: 'GLENTRY',
  query: "BATCH_DATE >= '03-01-2018' AND BATCH_DATE <= '03-15-2018'",
  fields: '*',
  pagesize: 100
}

request.send

This would make create calls the look like:

request = IntacctRuby::Request.new(REQUEST_OPTS)

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

request.create object_type: :PROJECT, parameters: {
  PROJECTID: '1',
  NAME: 'Get Chewie a Haircut',
  PROJECTCATEGORY: 'Improve Wookie Hygene',
  CUSTOMERID: '1',
  SHAMPOO: 'true', # a custom field
  BLOWDRY: 'false' # a custom field
}

request.send

This change would happen here https://github.com/privateprep/intacct-ruby/blob/master/lib/intacct_ruby/function.rb#L18

def initialize(function_type, object_type:, parameters: {})

What do you think? Let me know when possible.
Will hold off with merging until I hear back.

@bezoar17
Copy link
Copy Markdown
Contributor Author

Using keyword arguments throughout the gem is the more elegant and better solution. I agree to this. But I think that this should go as a separate PR.
Because, this is a bug I think this quick patch should go through.
I will be happy to pick up introducing keyword arguments next.
Let me know if this works

@danielrose7
Copy link
Copy Markdown
Contributor

Agree with you about best strategy,

Certainly open to your help with keywords! If you want to break it up, also happy to collaborate

@danielrose7 danielrose7 merged commit 6962a9d into privateprep:master Mar 19, 2018
@danielrose7
Copy link
Copy Markdown
Contributor

danielrose7 commented Mar 20, 2018

@bezoar17 1.8.2 has your changes! Thanks for the PR

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.

2 participants