Skip to content

add Staff.list IGNT-3540#2

Merged
bedrock-adam merged 12 commits intomasterfrom
staff/list
Apr 27, 2020
Merged

add Staff.list IGNT-3540#2
bedrock-adam merged 12 commits intomasterfrom
staff/list

Conversation

@bedrock-adam
Copy link
Contributor

@bedrock-adam bedrock-adam commented Apr 24, 2020

Intention (What)

expect(
  Staff.list(api_key: api_key, account_key: account_key)
).to include(
  Staff.build(
    name: "Dev Testing",
    email: "dev@practiceignition.com",
    uuid: "8c79202e-caf8-4a12-a3d7-ac338e50741f"))

Motivation (Why)

WorkflowMAX and Xero Practice Manager API v3 are out and we need to upgrade soon.

Implementation (How)

  1. Use Tony's Connection to get http response
  2. Use Ox to convert xml body string to hash
  3. Use PORO's with kwargs to define model structure

end
end
end
end
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Won't claim to be an expert at writing SAX parsers 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

have kept the puts in there as they're pretty useful for debugging.

end
end
end
end
Copy link
Contributor Author

@bedrock-adam bedrock-adam Apr 24, 2020

Choose a reason for hiding this comment

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

Integration with VCR soon

end
end
end
end
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could keep it as a PORO or use something like dry types? Open to ideas.

We can also delay a decision too - shouldn't break any tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am happy keeping it as a PORO for now...

@bedrock-adam bedrock-adam marked this pull request as ready for review April 24, 2020 05:56
@bedrock-adam bedrock-adam requested review from a team and abreckner April 24, 2020 05:56
def list(api_key:, account_key:)
key = Base64.strict_encode64("#{api_key}:#{account_key}")

http_response = Faraday
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @subskii it looks like there is functionality in here that should be in the connector? Is this code intended to be temporary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeap this is just to get the ball rolling 😉.

Let's swap it out with your code in a follow up PR 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah cool!

xpm_ruby.gemspec Outdated
spec.require_paths = ["lib"]

spec.add_runtime_dependency("faraday", "~> 1")
spec.add_runtime_dependency("ox", "~> 2.13")
Copy link
Contributor

Choose a reason for hiding this comment

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

Any particular reason to use ox over nokogiri? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Came across a somewhat older article praising it's memory usage. Seems a lot lighter and well optimised based on reviews.

Check out the description.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah ok cool. I will check it out.
:)

@bedrock-adam bedrock-adam requested a review from a team April 24, 2020 06:25
@bedrock-adam bedrock-adam changed the title add Staff.list add Staff.list IGNT-3540 Apr 26, 2020
.get(endpoint: "staff.api/list")

case response.status
when 401
Copy link
Contributor

Choose a reason for hiding this comment

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

For later (not this PR), maybe we should create an ConnectionErrors module (might help us DRY up the code a bit)? What do you think @subskii ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess let's see how this develops. :)

Copy link
Contributor

@abreckner abreckner left a comment

Choose a reason for hiding this comment

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

Great stuff!

@bedrock-adam bedrock-adam requested a review from a team April 27, 2020 00:13
@bedrock-adam bedrock-adam merged commit 9655211 into master Apr 27, 2020
@bedrock-adam bedrock-adam deleted the staff/list branch April 27, 2020 01:18
lankz pushed a commit that referenced this pull request Jun 10, 2025
* add Staff.list

* fix model namespace

* remove extra line

* drop sax parser

* remove parser spec

* optimise

* fix payroll_code

* add dry typesa

* Revert "add dry typesa"

This reverts commit 05de0bc.

* superclass errors + add api_url

* update to use Connection
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants