[WIP] ActionsBuilder #119
[WIP] ActionsBuilder #119
Conversation
|
This pull request is not mergeable. Please rebase and repush. |
|
Checked commits jntullo/manageiq-api@8214edd~...cd6ace1 with ruby 2.3.3, rubocop 0.47.1, and haml-lint 0.20.0 |
|
@miq-bot assign @imtayadeway |
There was a problem hiding this comment.
@jntullo this is looking great! I left a few comments above, LMK if any are problematic or if you want to 🍐
|
|
||
| def user_role_allows?(action_identifier) | ||
| return true unless action_identifier | ||
| User.current_user.role_allows?(:identifier => action_identifier) |
There was a problem hiding this comment.
WDYT about passing the current user into the constructor, so that this class doesn't need to know about the User class? It should make it easier to test, for one 😁
| RSpec.describe Api::ActionsBuilder do | ||
| let(:role) { FactoryGirl.create(:miq_user_role) } | ||
| let(:group) { FactoryGirl.create(:miq_group, :miq_user_role => role) } | ||
| let(:user) { FactoryGirl.create(:user, :miq_groups => [group]) } |
There was a problem hiding this comment.
If you instead pass the user in, you could use a stub instead of creating these three objects (and accompanying role-updating code):
user = instance_double("User")
# ...
allow(user).to receive(:role_allows?) do |arg|
%w(service_edit service_create).include?(arg) #etc
endI think this is OK because we should have good confidence that #role_allows does the right thing
| let(:role) { FactoryGirl.create(:miq_user_role) } | ||
| let(:group) { FactoryGirl.create(:miq_group, :miq_user_role => role) } | ||
| let(:user) { FactoryGirl.create(:user, :miq_groups => [group]) } | ||
| let(:request) { instance_double(Api::BaseController::Parser::RequestAdapter) } |
There was a problem hiding this comment.
We should really move the RequestAdapter 😉
Nit picking here, but if you enclose this class in quotes it'll defer the loading of it until it's required later
| attr_reader :config, :href, :type, :cspec, :collection, :subcollection, :resource | ||
|
|
||
| def initialize(request, href, type, resource = nil) | ||
| @config = CollectionConfig.new |
There was a problem hiding this comment.
Similarly, you could pass this into the constructor (but maybe leaving it as the default), meaning you could test this with a fake config that conforms to the same structure. I think this would be OK since there should be good coverage that this works in general in the request specs, but would enable you to create edge cases that are not easy to find in the wild, and also decouples the tests from the specifics of the api.yml. Small changes there could cause these tests to fail, which would be unfortunate.
| User.current_user = user | ||
| end | ||
|
|
||
| describe '#collection_actions' do |
There was a problem hiding this comment.
It looks like the only public method is #actions - perhaps these example groups need to be grouped under a single #actions group?
| @@ -128,7 +128,7 @@ def expect_result_to_have_custom_actions_hash | |||
|
|
|||
| expect_result_to_have_keys(%w(id href actions)) | |||
| action_specs = response.parsed_body["actions"] | |||
| expect(action_specs.size).to eq(1) | |||
| expect(action_specs.size).to eq(3) | |||
There was a problem hiding this comment.
It looks like this behavior changed during a refactoring. Nit, but it might help to break up this commit into the refactoring part and the part that introduced the change in behavior.
| @@ -822,108 +822,6 @@ def create_vms_by_name(names) | |||
| end | |||
|
|
|||
| describe "Querying resources" do | |||
| it "does not return actions if not entitled" do | |||
There was a problem hiding this comment.
I think if we have good unit tests for the extracted object, we probably don't need exhaustive edge cases tested at this level 👍 I would however leave one or two general cases to show that everything works when integrated. WDYT?
|
This pull request is not mergeable. Please rebase and repush. |
Breaking out a lot of the action logic that is held in the
rendererinto anActionsBuilder. We have a lot of tests in random places for actions, so this will allow us to centralize those tests as well as reduce what the renderer does.@miq-bot add_label wip, refactoring