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

Fixes #7307 - Refactor errata/package/package group APIs #4629

Merged
merged 1 commit into from Sep 9, 2014
Merged

Fixes #7307 - Refactor errata/package/package group APIs #4629

merged 1 commit into from Sep 9, 2014

Conversation

daviddavis
Copy link
Contributor

Original task was to add missing params to the package group API but noticing some similarities among packages/errata/package groups APIs, I made the code more DRY as well.

@daviddavis daviddavis changed the title [DO NOT REVIEW] Fixes #7307 - Refactor errata/package/package group APIs Fixes #7307 - Refactor errata/package/package group APIs Sep 3, 2014
@ehelms
Copy link
Member

ehelms commented Sep 8, 2014

Testing this

param :repository_id, :number, :desc => N_("repository identifier")
param_group :search, Api::V2::ApiController
def index
super
Copy link
Member

Choose a reason for hiding this comment

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

by having 2 different index methods doesn't the apipie doc for the index action get included twice? (or am i remembering incorrectly).

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's when you include a module that uses alias_method_chain. I double checked and the apidoc is correct (it doesn't have a content_view_filter_id param or two sets of paths, etc)

@ehelms
Copy link
Member

ehelms commented Sep 8, 2014

Only weirdness I noticed was if I hit a package group filter with the following:

/katello/api/v2/content_views/2/filters/1/errata

I get

    "displayMessage": "undefined method `erratum_rules' for #<Katello::ContentViewPackageGroupFilter:0x007f45603cd7a8>",

before_filter :find_content_resource, :only => [:show]
end

extend ::Apipie::DSL::Concern
Copy link
Member

Choose a reason for hiding this comment

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

What about putting the extend's together to make them easier to find?

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 got this from the taxonomies module in foreman. I tried moving both extend calls above the included block but then I got:

wrong number of arguments (0 for 1)
apipie-rails (0.2.4) lib/apipie/dsl_definition.rb:353:in `included`
/home/dadavis/Projects/katello/app/controllers/katello/concerns/api/v2/repository_content_controller.rb:19:in `&lt;module:RepositoryContentController&gt;&#x27;
/home/dadavis/Projects/katello/app/controllers/katello/concerns/api/v2/repository_content_controller.rb:15:in `&lt;module:Concerns&gt;&#x27;
/home/dadavis/Projects/katello/app/controllers/katello/concerns/api/v2/repository_content_controller.rb:14:in `&lt;module:Katello&gt;&#x27;
/home/dadavis/Projects/katello/app/controllers/katello/concerns/api/v2/repository_content_controller.rb:13:in `&lt;top (required)&gt;&#x27;

When I moved both after:

wrong number of arguments(0 for 1)
/home/dadavis/Projects/katello/app/controllers/katello/concerns/api/v2/repository_content_controller.rb:16:in `included&#x27;
/home/dadavis/Projects/katello/app/controllers/katello/concerns/api/v2/repository_content_controller.rb:16:in `&lt;module:RepositoryContentController&gt;&#x27;
/home/dadavis/Projects/katello/app/controllers/katello/concerns/api/v2/repository_content_controller.rb:15:in `&lt;module:Concerns&gt;&#x27;
/home/dadavis/Projects/katello/app/controllers/katello/concerns/api/v2/repository_content_controller.rb:14:in `&lt;module:Katello&gt;&#x27;
/home/dadavis/Projects/katello/app/controllers/katello/concerns/api/v2/repository_content_controller.rb:13:in `&lt;top (required)&gt;&#x27;

Looks like it needs to be this way.

@daviddavis
Copy link
Contributor Author

Added a check for the filter type so it checks filter type vs content. Also added a test.

@ehelms
Copy link
Member

ehelms commented Sep 9, 2014

ACK

daviddavis pushed a commit that referenced this pull request Sep 9, 2014
Fixes #7307 - Refactor errata/package/package group APIs
@daviddavis daviddavis merged commit f2910e1 into Katello:master Sep 9, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants