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 GlobalStorage API support #4

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open

Conversation

izikaj
Copy link

@izikaj izikaj commented Sep 6, 2023

In order to add new GlobalStorage API support there are such updates:

  • Added Monday::Storage class to work with the new API
  • Updated code style for more readability
  • Eased some dependency versions (Faraday is now allowed to have version 2.x)
  • Removed excessive conversions to JSON string
  • The default behavior of the internal Faraday client is set to parse JSON responses automatically
  • Organized existing tests
  • Added tests to cover Monday::Storage behavior
  • Updated readme.

@izikaj
Copy link
Author

izikaj commented Sep 6, 2023

This update has a breaking change - the Monday::Client API requests are now returning hashes instead of strings (so we don't need to do JSON.parse(JSON.parse(response)) anymore).
So we probably should bump the version to 1.0.0, is it ok, or do you prefer 0.2.0?

@izikaj izikaj marked this pull request as ready for review September 6, 2023 06:03
.rspec_status Outdated
Copy link
Author

Choose a reason for hiding this comment

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

Do we really need the file in repo?

MONDAY_API_URL = "#{MONDAY_PROTOCOL}://api.#{MONDAY_DOMAIN}/v2".freeze
MONDAY_OAUTH_URL = "#{MONDAY_PROTOCOL}://auth.#{MONDAY_DOMAIN}/oauth2/authorize".freeze
MONDAY_OAUTH_TOKEN_URL = "#{MONDAY_PROTOCOL}://auth.#{MONDAY_DOMAIN}/oauth2/token".freeze
MONDAY_STORAGE_URL = "#{MONDAY_PROTOCOL}://apps-storage.#{MONDAY_DOMAIN}/app_storage_api/v2".freeze
Copy link
Author

Choose a reason for hiding this comment

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

Here is the new API endpoint

#
# @param token [String] The users access token (permanent or short-lived) used for API requests.
# @param connection [Faraday::Connection] An existing Faraday connection to be used for the requests.
def initialize(token: nil, api: nil, conn: nil)
Copy link
Author

Choose a reason for hiding this comment

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

Initialization designed to be the same as for Monday::Client

full_url = "#{api_domain}/#{URI.encode_www_form_component(key)}"
return full_url unless shared

"#{full_url}?shareGlobally=true"
Copy link
Author

Choose a reason for hiding this comment

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

This query string came from the original JS library

README.md Outdated
Copy link
Author

Choose a reason for hiding this comment

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

this should resolve the documentation issue


client.response :raise_error
client.response :json
Copy link
Author

Choose a reason for hiding this comment

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

We should parse JSON in the middleware, to avoid doing this by ourselves


def api_request(url, conn, data, token)
res = conn.post(url, data, 'Authorization' => token, 'Content-Type' => 'application/json')
res.body
Copy link
Author

Choose a reason for hiding this comment

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

Here we have a valid ruby hash, we do not need to convert it to JSON string again

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.

2 participants