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 support for API version 5 #114

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

Conversation

peret
Copy link

@peret peret commented Apr 24, 2022

Closes #100

I did my best to make the changes backwards-compatible and preserve the public API. However, I think this can still be considered a breaking change for some people. Especially if they did not specify an API version explicitly in the configuration, they will be changed to version 5 when upgrading the gem, which will likely break existing code.

There's also a subtle difference in how the API classes are loaded. The correct classes will be included in the API-module based on the configured API version, which means the API module will not be accessible before you configured the gem.

I cannot guarantee that I didn't miss any fields, since the Alchemer API documentation is a little unwieldy, but I hope this should be a good starting point.

peret and others added 15 commits April 18, 2022 10:43
* teamname on AccountTeam is specific to v4. It's called team_name in v5.
* answers on Response seems to be a v4-field. In v5 we get survey_data
  instead.
Separate classes into v4- and v5-submodules. Load resource files based
on configuration.
Changes are mostly copied from mkaydev's original change set.
Parsed answers for Radio- and Checkbox-questions expose the same
interface, i.e. there's an options-field that contains all the selected
options for that answer.
@peret
Copy link
Author

peret commented Apr 24, 2022

Not sure how to handle the Travis CI failure. I can run the specs successfully on my machine using ruby-2.7.1. However, on my system "bundle install" installs activesupport-6.1.5, not activesupport-7.0.2.3. Should I restrict the activesupport version in the gemspec, or find a different workaround?

@ylecuyer
Copy link
Owner

Not sure how to handle the Travis CI failure. I can run the specs successfully on my machine using ruby-2.7.1. However, on my system "bundle install" installs activesupport-6.1.5, not activesupport-7.0.2.3. Should I restrict the activesupport version in the gemspec, or find a different workaround?

I checked and found the root cause, I'll update the project with the fix and move from travis CI to github actions at the same time. Thanks for the head ups

@@ -17,4 +17,5 @@

path = File.join(File.expand_path(File.dirname(__FILE__)), 'survey_gizmo')
Dir["#{path}/*.rb"].each { |f| require f }
Dir["#{path}/**/*.rb"].each { |f| require f }
Copy link
Owner

Choose a reason for hiding this comment

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

Here we need to keep the **/*.rb otherwise the SurveyGizmo::API won't be required and you wont be able to use thing like: SurveyGizmo::API::Survey.all(all_pages: true)

You can give it a try with an new rails project:

rails new -d sqlite3 test-survey-gizmo-rails

and in the Gemfile point at your local version of the gem

gem 'survey-gizmo-ruby' #, path: '../survey-gizmo-ruby'

Then if you do bundle exec rails c you'll have the following error:

ylecuyer@inwin:~/Projects/test-survey-gizmo-rails$ bundle exec rails c
Running via Spring preloader in process 7319
Loading development environment (Rails 6.1.5)
irb(main):001:0> SurveyGizmo::API::Survey.all(all_pages: true)
Traceback (most recent call last):
        1: from (irb):1
NameError (uninitialized constant SurveyGizmo::API)
irb(main):002:0> 

Instead of:

ylecuyer@inwin:~/Projects/test-survey-gizmo-rails$ bundle exec rails c
Running via Spring preloader in process 7463
Loading development environment (Rails 6.1.5)
irb(main):001:0> SurveyGizmo::API::Survey.all(all_pages: true)
Traceback (most recent call last):
        1: from (irb):1
RuntimeError (Not configured!)

which is expected

@ylecuyer
Copy link
Owner

Thanks for the PR, I posted an inline comment and I could also tell you to try avoiding duplicating v4 and v5 classes, afaics changes are mainly on attributes and we could have v4/v5 attributes on the resources at the same time without having them conflicting.

For 2 or 3 files, I saw you changed some logic, maybe you could try adding the if configuration.v5? there and maybe give example with an inline comment on the PR explaining why those changes on the logic is needed (this will help for the review)

You'll also have to add a new line on the changelog and probably edit the readme a little.

@ylecuyer
Copy link
Owner

ylecuyer commented Aug 5, 2023

Just saw you made another branch with the feedback (https://github.com/playtestcloud/survey-gizmo-ruby/tree/version-5-pr-feedback) do you want to open another PR from this branch to upstream your changes?

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.

Version 5
2 participants