system groups - UI - allow user to change the env/view for systems in a group #1837

Merged
merged 6 commits into from Apr 3, 2013

Projects

None yet

2 participants

@bbuckingham
Member

This PR contains modifications to allow a user to change (via UI) the environment and content view for all users in a system group. This PR also contains changes to the group's system list to show the current env/view associated with each system.

To change the env/view, the user would:

  • go to System Groups -> -> Systems
  • click the 'Edit' link
  • select environment/content view and click 'Save'

Since this will change the values for all systems in the group, the user is presented with a confirmation dialog before proceeding with the change.

As part of these changes, I created 'env_content_view_selector.js' which can hopefully be reused for similar scenarios in the UI (e.g. Systems & Activation Keys create). I've also done some minor refactoring of those other areas to centralized the definition of some strings and view helpers. (Will do any further refactoring to use this common js as part of a separate PR.)

bbuckingham added some commits Apr 1, 2013
@bbuckingham bbuckingham system groups - update system's list to include env and view
Adding a little more info to the system's list.
37263f2
@bbuckingham bbuckingham activation keys / systems - minor refactor to allow for reuse on syst…
…em groups

For content views, we will be updating system groups to allow
the admin to change the env/view for all systems in a group.
This will use a similar env/view widget to what is currently
used in both activation keys and systems.  In preparation for
that, this commit is moving some of the logic to be common.

This commit primarily addresses:
1. moves the definition of 'no content view' - placing it in common_i18n
   and application_helper
2. moves the logic for obtaining the content view select labels
   to application_helper
f3a46a2
@bbuckingham bbuckingham ctivation keys / systems - mv i18n.update_view to _common_i18n partial
Renaming update_view to select_content_view and moving it to
the _common_i18n.html.haml partial.
b524f42
@bbuckingham bbuckingham activation keys - test fix fbbae0c
@bbuckingham bbuckingham system groups - ui - allow user to change env/view for systems in a g…
…roup

This commit contains changes to allow a user to navigate to
a system group and then 'edit' the environment/content view
for all systems in that group.

As part of the change, created env_content_view_selector.js, which
we may be able to leverage in other places in the UI where similar
behavior is used (e.g. activation key & system 'create').
ac176ca
@daviddavis daviddavis and 1 other commented on an outdated diff Apr 3, 2013
src/app/controllers/system_groups_controller.rb
@@ -39,6 +41,8 @@ def rules
:auto_complete=>any_readable,
:add_systems=> edit_perm,
:remove_systems=>edit_perm,
+ :edit_systems=>edit_systems_perm,
+ :update_systems=>edit_systems_perm,
@daviddavis
daviddavis Apr 3, 2013 Member

In param_rules, you use spaces around your hash rockets, but here you don't. Any reason?

Personally, I prefer spaces around them since it makes it easier to read (and also a space after the lambda keyword).

@bbuckingham
bbuckingham Apr 3, 2013 Member

The only reason was to be consistent within the blocks of code where the lines were added. That said, I can create a commit to change the code, but will change it throughout the controller to be consistent (i.e. not just the lines I added with this PR).

@daviddavis daviddavis and 1 other commented on an outdated diff Apr 3, 2013
src/public/javascripts/env_content_view_selector.js
+ including the implied warranties of MERCHANTABILITY,
+ NON-INFRINGEMENT, or FITNESS FOR A PARTICULAR PURPOSE. You should
+ have received a copy of GPLv2 along with this software; if not, see
+ http://www.gnu.org/licenses/old-licenses/gpl-2.0.txt.
+ */
+KT.env_content_view_selector = (function(){
+ var settings,
+
+ initialize = function(params) {
+ /**
+ * params:
+ * override_save // (default: true) if false, perform save on form submit
+ * override_cancel // (default: true) if false, perform cancel when cancel button clicked
+ * cv_change_cb() // callback function to perform custom logic after user selects content view
+ * save_success_cb() // callback function to perform custom logic after a successful save
+ * save_error_cb // callback function to perform custom logic after an unsuccessful save
@daviddavis
daviddavis Apr 3, 2013 Member

Why no parens?

@bbuckingham
bbuckingham Apr 3, 2013 Member

Unintentional, will add.

@daviddavis daviddavis and 1 other commented on an outdated diff Apr 3, 2013
src/public/javascripts/env_content_view_selector.js
+/**
+ Copyright 2013 Red Hat, Inc.
+
+ This software is licensed to you under the GNU General Public
+ License as published by the Free Software Foundation; either version
+ 2 of the License (GPLv2) or (at your option) any later version.
+ There is NO WARRANTY for this software, express or implied,
+ including the implied warranties of MERCHANTABILITY,
+ NON-INFRINGEMENT, or FITNESS FOR A PARTICULAR PURPOSE. You should
+ have received a copy of GPLv2 along with this software; if not, see
+ http://www.gnu.org/licenses/old-licenses/gpl-2.0.txt.
+ */
+KT.env_content_view_selector = (function(){
+ var settings,
+
+ initialize = function(params) {
@daviddavis
daviddavis Apr 3, 2013 Member

Why is this indented?

@bbuckingham
bbuckingham Apr 3, 2013 Member

No particular reason. It may have been from ide.

@daviddavis
Member

Good job. ACK pending some minor comments.

@bbuckingham bbuckingham system groups - address style comments from pull request 1837 review
Addresses a few minor style comments from 1837:
- spacing around => in controller
- spacing after lambda in controller
- inclusion of () on selector callback function
- indentation in *selector.js
e036360
@daviddavis
Member

ACK

@bbuckingham
Member

I have a need to get this code in to master and another branch. Since tests are passing and I've gotten an ACK, I am going to proceed with merging. If, however, folks have any additional comments, feel free to send them to me and I'll get them incorporated.

@bbuckingham bbuckingham merged commit 7af81a5 into Katello:master Apr 3, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment