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

Added CRUD support for Automate Domains. #26

Merged
merged 4 commits into from
Jun 18, 2014

Conversation

h-kataria
Copy link
Contributor

  • Added support to Add/Edit/Delete Automate Domains. Moved some of the methods to private area and changed existing code that was being used to add/edit namespaces to be used for add/edit of domains.
  • Added support to change priority of Automate Domains.
  • Added support to allow users to local/unlock domains. This feature is protected by RBAC, so only users that have access to these features will be able to see these buttons.
  • Added product features for the new buttons.
  • Changed role_allows? method in user model to check for allows_any? feature when a hidden feature comes in.
  • Added spec tests to verify, lock/unlock and priority order change features.
  • Fixed any broken spec tests in application_helper specs, deleted any AE related spec tests that aren't valid any more.
  • Fixed an issue where correct tabs were not shown as inactive tabs on edit screen.

Issue #2045

@dclarizio please review/test.

@Fryguy
Copy link
Member

Fryguy commented Jun 16, 2014

@martinpovolny @mkanoor @gmcculloug @tinaafitz Please review.

when 'props' then @edit[:rec_id].nil? ? 'create' : 'update'
else @sb[:action] == 'miq_ae_field_seq' ? 'fields_seq_edit' : 'update_fields'
end
end
Copy link
Member

Choose a reason for hiding this comment

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

I would instantiate a variable, such as
verb = @edit[:rec_id].nil? 'create' : 'update'

and then make a hash
{ 'instances' => '_instance', 'props' => '' ...etc}
then calculate the value of the action_url from the 2 parts (hash lookup + verb)\

that would make the eeeextra loooooong method a bit shorter

and then I would put all that stuff into a separate method such as
action_url = calculate_action_url(nodes.first,...) or something like that

@h-kataria
Copy link
Contributor Author

@martinpovolny @mkanoor ready for re-review, made suggested changes.

- Added support to Add/Edit/Delete Automate Domains. Moved some of the methods to private area and changed existing code that was being used to add/edit namespaces to be used for add/edit of domains.
- Added support to change priority of Automate Domains.
- Added support to allow users to local/unlock domains. This feature is protected by RBAC, so only users that have access to these features will be able to see these buttons.
- Added product features for the new buttons.
- Changed role_allows? method in user model to check for allows_any? feature when a hidden feature comes in.
- Added spec tests to verify, lock/unlock and priority order change features.
- Fixed any broken spec tests in application_helper specs, deleted any AE related spec tests that aren't valid any more.
- Fixed an issue where correct tabs were not shown as inactive tabs on edit screen.

Issue ManageIQ#2045
- Some cleanup in the code where action_url is being set, moved that into its own method.
- More cleanup in the area where attributes are being set for MiqAeField. Changed name of the form field for substitute column so looping over form fields in controller to set MiqAeField attributes gets easier.
- Updated spec test to use miq_ae_domain_enabled and miq_ae_domain_disabled factories.
- Fixed an issue found with Domain delete during testing.

Issue ManageIQ#2045
@h-kataria
Copy link
Contributor Author

@martinpovolny made suggested changes

@martinpovolny
Copy link
Member

@h-kataria : I wonder if you have Rubocop setup locally? If not I'd suggest that you install it and take the config that we have here so that you can address Rubocop complaints comfortably. Most of the above are actually quite sane. If you need some help with Rubocop, ask me anytime!

@edit[:rec_id] = @ae_ns.id || nil
@edit[:key] = "aens_edit__#{@ae_ns.id || "new"}"
@edit[:new][:ns_name] = @ae_ns.name
@edit[:new][:ns_description] = @ae_ns.description
Copy link
Member

Choose a reason for hiding this comment

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

could we, please, make the above look like

@edit = {
 :new => {
    :ns_name => ....
    ...
  },
  :key =>
 ...

should be less characters and actually more readable

- Added Lock/Unlock as a parent feature and made Lock and Unlock features as hidden features under new feature so only a single feature shows up in the Access Control to toggle between Lock/Unlock.

Issue ManageIQ#2045
@miq-bot
Copy link
Member

miq-bot commented Jun 18, 2014

Checked commits h-kataria@f7e08ad .. h-kataria@d003bb3 with rubocop 0.21.0
8 files checked, 17 offenses detected

vmdb/app/controllers/miq_ae_class_controller.rb

vmdb/app/models/user.rb

vmdb/spec/controllers/miq_ae_class_controller_spec.rb

  • Style - Line 83, Col 49 - Blocks - Avoid using {...} for multi-line blocks.

@dclarizio
Copy link

@h-kataria @martinpovolny We will address the remaining rubocop comments in a follow up commit. I am merging this to get the functionality into the next build.

dclarizio pushed a commit that referenced this pull request Jun 18, 2014
Added CRUD support for Automate Domains.
@dclarizio dclarizio merged commit 013e7f3 into ManageIQ:master Jun 18, 2014
@h-kataria h-kataria deleted the ae_domains_crud_ui branch June 23, 2014 20:45
h-kataria added a commit to h-kataria/manageiq that referenced this pull request Jun 24, 2014
- Addressed rubocop comments from the previous PR ManageIQ#26 (comment).
- Fixed a typo in a toolbar button title/text
simaishi pushed a commit that referenced this pull request Jan 11, 2017
Allow the use of all timezones in the console
(cherry picked from commit 7e869f18c3cafc7f2ee691b7d0d5456c5ec2b97a)

https://bugzilla.redhat.com/show_bug.cgi?id=1402118
simaishi pushed a commit that referenced this pull request May 12, 2017
…ices_in_one_call

Retrieve host storage devices host-by-host
(cherry picked from commit 84f5b69d7a15cae2033598f2ff43c8197adbd1aa)

https://bugzilla.redhat.com/show_bug.cgi?id=1450526
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.

6 participants