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

Add access_scopes attribute to session object #843

Merged
merged 1 commit into from
Feb 24, 2021

Conversation

NabeelAhsen
Copy link
Contributor

@NabeelAhsen NabeelAhsen commented Feb 23, 2021

Related: Shopify/shopify_app#1192

This PR introduces an access_scopes attribute to the Session object.

Things to focus on

  1. Do we determine the validity of a session through this attribute?
  2. Is there immediate benefit in returning a ShopifyAPI::ApiAccess object? Will a string suffice?
  3. Is this level of error handling needed?

@NabeelAhsen NabeelAhsen force-pushed the session/add-access_scopes-attribute branch from 45cfb83 to 1da3feb Compare February 23, 2021 19:57
@NabeelAhsen NabeelAhsen self-assigned this Feb 23, 2021
@NabeelAhsen NabeelAhsen marked this pull request as ready for review February 23, 2021 19:58
@NabeelAhsen NabeelAhsen requested a review from a team as a code owner February 23, 2021 19:58
@NabeelAhsen NabeelAhsen changed the title Add access_scopes attribute Add access_scopes attribute to session object Feb 23, 2021
@rezaansyed
Copy link
Contributor

  1. Do we determine the validity of a session through this attribute?
  2. Is there immediate benefit in returning a ShopifyAPI::ApiAccess object? Will a string suffice?
  3. Is this level of error handling needed?
  1. Not sure what you mean here with this. But this is only a value object used to encompass the access token (and now access scopes)
  2. ShopifyAPI::ApiAccess should be the preferred approach here. Anyone using this object has the ability to compare two objects for equality and subsets.
  3. No. access_scopes should be nil if not specified.

Copy link
Contributor

@rezaansyed rezaansyed left a comment

Choose a reason for hiding this comment

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

🎩 LGTM 👍🏼

lib/shopify_api/session.rb Outdated Show resolved Hide resolved
lib/shopify_api/session.rb Outdated Show resolved Hide resolved
lib/shopify_api/session.rb Outdated Show resolved Hide resolved
end

test "session instantiation raises error if bad access scopes are provided" do
assert_raises do
Copy link
Contributor

Choose a reason for hiding this comment

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

What does it raise?

Copy link
Contributor

Choose a reason for hiding this comment

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

Currently it's NoMethodError because the strip method doesn't exist on the object passed in object passed in. I can specify NoMethodError

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we have a custom error for ApiAccess to throw when an invalid input is passed in?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that because specifying api_access is opt-in, it's up to the user to specify good input. In this case, building a new session will either:

  • parse access_scopes correctly or
  • throw some error (automatically an invalid session)

For this test, we know that it's NoMethodError because strip doesn't exist on the hash, but really the bad input could throw anything.

Copy link
Contributor Author

@NabeelAhsen NabeelAhsen 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 addressing comments @rezaansyed!

lib/shopify_api/session.rb Outdated Show resolved Hide resolved
@NabeelAhsen NabeelAhsen force-pushed the session/add-access_scopes-attribute branch from 4aab6ef to 63108ed Compare February 24, 2021 15:54
Copy link
Contributor

@mkevinosullivan mkevinosullivan left a comment

Choose a reason for hiding this comment

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

👍🏻

@NabeelAhsen NabeelAhsen merged commit 4d64fb1 into master Feb 24, 2021
@NabeelAhsen NabeelAhsen deleted the session/add-access_scopes-attribute branch February 24, 2021 20:22
@NabeelAhsen NabeelAhsen temporarily deployed to rubygems February 24, 2021 20:56 Inactive
@shopify-shipit shopify-shipit bot temporarily deployed to rubygems April 4, 2022 13:35 Inactive
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.

4 participants