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.

Sign up for GitHub

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

Jump to bottom

Add new Tagging instead of RBAC tree for groups #5482

Merged
merged 11 commits into from
May 6, 2019
Merged

Add new Tagging instead of RBAC tree for groups #5482

merged 11 commits into from
May 6, 2019

Conversation

PanSpagetka
Copy link
Contributor

@PanSpagetka PanSpagetka commented Apr 24, 2019 •
edited
Loading

Add new Tagging instead of RBAC tree for groups

Links [Optional]

ManageIQ/react-ui-components#105

Steps for Testing/QA [Optional]

Go to Ops -> Account Settings -> Groups
Old:
Screenshot from 2019-04-30 15-18-46
Screenshot from 2019-04-30 15-18-08
New:
Screenshot from 2019-04-30 15-12-35
Screenshot-from-2019-05-02-11-44-13

@skateman

@PanSpagetka
Copy link
Contributor Author

@miq-bot add_reviewer @skateman

@miq-bot miq-bot requested a review from skateman April 24, 2019 08:03
@PanSpagetka PanSpagetka force-pushed the add-tagging-ops-rbac branch 3 times, most recently from 9bff3fd to 2afe3f4 Compare April 24, 2019 08:14
@skateman
Copy link
Member

Depends on ManageIQ/react-ui-components#99

@skateman
Copy link
Member

@PanSpagetka can you please update the react-ui-components version to 0.11.11, ideally in this PR.

@skateman
Copy link
Member

skateman commented Apr 24, 2019 •
edited
Loading

I think the tags word is not there enough times :trollface: 🤣 but aside the joke it shouldn't be there twice, also it looks weird if there are no assigned tags at all 😕
Screenshot from 2019-04-24 13-20-28

When I tried to add some tags and clicked the save button, got this error:

Error caught: [NoMethodError] undefined method `description' for nil:NilClass
/home/skateman/Repositories/ManageIQ/manageiq-ui-classic/app/controllers/ops_controller/ops_rbac.rb:923:in `block in rbac_group_right_tree'
/home/skateman/.rbenv/versions/2.4.5/lib/ruby/gems/2.4.0/gems/activerecord-5.0.7.2/lib/active_record/relation/delegation.rb:38:in `map'
/home/skateman/.rbenv/versions/2.4.5/lib/ruby/gems/2.4.0/gems/activerecord-5.0.7.2/lib/active_record/relation/delegation.rb:38:in `map'
/home/skateman/Repositories/ManageIQ/manageiq-ui-classic/app/controllers/ops_controller/ops_rbac.rb:921:in `rbac_group_right_tree'
/home/skateman/Repositories/ManageIQ/manageiq-ui-classic/app/controllers/ops_controller/ops_rbac.rb:898:in `rbac_group_get_details'
/home/skateman/Repositories/ManageIQ/manageiq-ui-classic/app/controllers/ops_controller/ops_rbac.rb:848:in `rbac_get_info'
/home/skateman/Repositories/ManageIQ/manageiq-ui-classic/app/controllers/ops_controller.rb:508:in `get_node_info'
/home/skateman/Repositories/ManageIQ/manageiq-ui-classic/app/controllers/ops_controller.rb:288:in `set_active_elements'
/home/skateman/Repositories/ManageIQ/manageiq-ui-classic/app/controllers/application_controller.rb:2177:in `build_accordions_and_trees'
/home/skateman/Repositories/ManageIQ/manageiq-ui-classic/app/controllers/ops_controller.rb:126:in `explorer'

@PanSpagetka
Copy link
Contributor Author

I think the tags word is not there too many times :trollface: rofl also it looks weird if there are no assigned tags at all confused
Screenshot from 2019-04-24 13-20-28

When I tried to add some tags and clicked the save button, got this error:

Error caught: [NoMethodError] undefined method `description' for nil:NilClass
/home/skateman/Repositories/ManageIQ/manageiq-ui-classic/app/controllers/ops_controller/ops_rbac.rb:923:in `block in rbac_group_right_tree'
/home/skateman/.rbenv/versions/2.4.5/lib/ruby/gems/2.4.0/gems/activerecord-5.0.7.2/lib/active_record/relation/delegation.rb:38:in `map'
/home/skateman/.rbenv/versions/2.4.5/lib/ruby/gems/2.4.0/gems/activerecord-5.0.7.2/lib/active_record/relation/delegation.rb:38:in `map'
/home/skateman/Repositories/ManageIQ/manageiq-ui-classic/app/controllers/ops_controller/ops_rbac.rb:921:in `rbac_group_right_tree'
/home/skateman/Repositories/ManageIQ/manageiq-ui-classic/app/controllers/ops_controller/ops_rbac.rb:898:in `rbac_group_get_details'
/home/skateman/Repositories/ManageIQ/manageiq-ui-classic/app/controllers/ops_controller/ops_rbac.rb:848:in `rbac_get_info'
/home/skateman/Repositories/ManageIQ/manageiq-ui-classic/app/controllers/ops_controller.rb:508:in `get_node_info'
/home/skateman/Repositories/ManageIQ/manageiq-ui-classic/app/controllers/ops_controller.rb:288:in `set_active_elements'
/home/skateman/Repositories/ManageIQ/manageiq-ui-classic/app/controllers/application_controller.rb:2177:in `build_accordions_and_trees'
/home/skateman/Repositories/ManageIQ/manageiq-ui-classic/app/controllers/ops_controller.rb:126:in `explorer'

No problem, we can more more Tags to Tagging. How about my favorite Tags? Or most recent Tags? Maybe we should include "like this Tag and share it to your Friends feature" :trollface:

@PanSpagetka PanSpagetka force-pushed the add-tagging-ops-rbac branch 2 times, most recently from f4c8100 to 7249537 Compare April 24, 2019 12:03
@skateman
Copy link
Member

Now I'm getting this error when I try to display a group:

FATAL -- : Error caught: [NoMethodError] undefined method `description' for nil:NilClass
/home/skateman/Repositories/ManageIQ/manageiq-ui-classic/app/controllers/ops_controller/ops_rbac.rb:923:in `block in rbac_group_right_tree'
/home/skateman/.rbenv/versions/2.4.5/lib/ruby/gems/2.4.0/gems/activerecord-5.0.7.2/lib/active_record/relation/delegation.rb:38:in `map'
/home/skateman/.rbenv/versions/2.4.5/lib/ruby/gems/2.4.0/gems/activerecord-5.0.7.2/lib/active_record/relation/delegation.rb:38:in `map'
/home/skateman/Repositories/ManageIQ/manageiq-ui-classic/app/controllers/ops_controller/ops_rbac.rb:921:in `rbac_group_right_tree'
/home/skateman/Repositories/ManageIQ/manageiq-ui-classic/app/controllers/ops_controller/ops_rbac.rb:898:in `rbac_group_get_details'
/home/skateman/Repositories/ManageIQ/manageiq-ui-classic/app/controllers/ops_controller/ops_rbac.rb:848:in `rbac_get_info'
/home/skateman/Repositories/ManageIQ/manageiq-ui-classic/app/controllers/ops_controller.rb:508:in `get_node_info'

@skateman
Copy link
Member

When editing a tag, you have too many save buttons:
Screenshot from 2019-04-25 11-10-55
Feels a little like:

@PanSpagetka
Copy link
Contributor Author

Depends on ManageIQ/react-ui-components#105

@PanSpagetka PanSpagetka force-pushed the add-tagging-ops-rbac branch 2 times, most recently from 847dcbb to dcfc155 Compare April 29, 2019 08:58
@skateman
Copy link
Member

select-tag-items

skateman
skateman suggested changes Apr 29, 2019
View reviewed changes
app/controllers/ops_controller/ops_rbac.rb Outdated
:filters => @filters,
:group => @group)
cats = Classification.categories.find_all do |c|
c if c.show || !%w[folder_path_blue folder_path_yellow].include?(c.name)
Copy link
Member

Choose a reason for hiding this comment

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

The Enumerable#find_all expects a boolean from the block, therefore, I don't understand why you want to use an if inside and return with the data itself. Also a select or a reject might be more understandable.

app/controllers/ops_controller/ops_rbac.rb Outdated
c if c.show || !%w[folder_path_blue folder_path_yellow].include?(c.name)
end
cats.sort_by! { |t| t.description.try(:downcase) } # Get the categories, sort by description
cats.delete_if { |c| c.read_only? || c.entries.empty? } # Remove categories that are read only or have no entries
Copy link
Member

Choose a reason for hiding this comment

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

You can combine this into the future select or reject described in my previous comment above.

app/controllers/ops_controller/ops_rbac.rb Outdated
end
}
end
filters = @edit&.dig(:new)&.dig(:filters) || @filters
Copy link
Member

Choose a reason for hiding this comment

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

fetch_path

app/controllers/ops_controller/ops_rbac.rb
else
arr << tag
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.

This might be a little complext, will think about it.

app/controllers/ops_controller/ops_rbac.rb Outdated
end

assigned_tags.uniq! { |tag| tag[:id] }
@tags = {:tags => @tags, :assignedTags => assigned_tags, :affectedItems => [@group.id]}
Copy link
Member

Choose a reason for hiding this comment

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

Recursively setting a variable increases code complexity, it counts as a double if you do it for an instance variable.

@tags = { :tags => @tags }

app/controllers/ops_controller/ops_rbac.rb Outdated
cat, tag = params[:id].split('cl-').last.split("_xx-") # Get the category and tag
# cat, tag = params[:id].split('cl-').last.split("_xx-") # Get the category and tag
cat = params[:cat]
tag = params[:val]
Copy link
Member

Choose a reason for hiding this comment

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

cat, tag = params.values_at(:cat, :val)

Copy link
Member

Choose a reason for hiding this comment

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

But I'd just simply use the params[:cat] and params[:tag] in the code below, no need for allocation if you don't use it elsewhere.

@PanSpagetka PanSpagetka changed the title Add tagging ops rbac Add new Tagging instead of RBAC tree for groups Apr 30, 2019
@PanSpagetka PanSpagetka force-pushed the add-tagging-ops-rbac branch 2 times, most recently from 1f392d2 to fd386db Compare April 30, 2019 13:14
@skateman
Copy link
Member

@miq-bot add_reviewer @epwinchell

@miq-bot miq-bot requested a review from epwinchell April 30, 2019 13:37
epwinchell
epwinchell approved these changes Apr 30, 2019
View reviewed changes
Copy link
Contributor

@epwinchell epwinchell left a comment

Choose a reason for hiding this comment

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

Looks fine. (the column layout/responsiveness is still messed up on react side, but that's for another PR)

@PanSpagetka PanSpagetka mentioned this pull request May 2, 2019
Closed
@PanSpagetka PanSpagetka force-pushed the add-tagging-ops-rbac branch 2 times, most recently from 7e5705c to 1b6473c Compare May 2, 2019 10:00
Hyperkid123
Hyperkid123 reviewed May 2, 2019
View reviewed changes
app/javascript/components/taggingWrapper.jsx Outdated
@@ -1,8 +1,9 @@
import React from 'react';
import { connect } from 'react-redux';
import { applyMiddleware } from 'redux'
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not used.

Hyperkid123
Hyperkid123 reviewed May 2, 2019
View reviewed changes
Copy link
Contributor

@Hyperkid123 Hyperkid123 left a comment

Choose a reason for hiding this comment

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

One small nitpick.

Can we add a test to the middleware? You can use the mock store and the middleware to it and check whether the api call was made or not.

@skateman
Copy link
Member

skateman commented May 3, 2019

@PanSpagetka please resolve the merge conflict

Hyperkid123
Hyperkid123 approved these changes May 6, 2019
View reviewed changes
Copy link
Contributor

@Hyperkid123 Hyperkid123 left a comment

Choose a reason for hiding this comment

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

Middleware test looks good

@miq-bot
Copy link
Member

miq-bot commented May 6, 2019

Checked commits PanSpagetka/manageiq-ui-classic@f95ecf8~...1fe8593 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
4 files checked, 1 offense detected

**

  • 💣 💥 🔥 🚒 - Linter/Haml - Linter::Haml STDERR:
warning: parser/current is loading parser/ruby23, which recognizes
warning: 2.3.6-compliant syntax, but you are running 2.3.3.
warning: please see https://github.com/whitequark/parser#compatibility-with-ruby-mri.

skateman
skateman approved these changes May 6, 2019
View reviewed changes
Copy link
Member

@skateman skateman left a comment

Choose a reason for hiding this comment

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

The Seal of Approval

@martinpovolny martinpovolny merged commit 81bc00d into ManageIQ:master May 6, 2019
@martinpovolny martinpovolny self-assigned this May 6, 2019
@martinpovolny martinpovolny added this to the Sprint 111 Ending May 13, 2019 milestone May 6, 2019
@skateman skateman mentioned this pull request May 6, 2019
Merged
@himdel himdel mentioned this pull request May 9, 2019
Closed
@PanSpagetka PanSpagetka mentioned this pull request May 15, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@skateman skateman skateman approved these changes

@epwinchell epwinchell epwinchell approved these changes

@Hyperkid123 Hyperkid123 Hyperkid123 approved these changes

Assignees

@martinpovolny martinpovolny

Labels
changelog/yes hammer/no react trees ux/review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@PanSpagetka @skateman @miq-bot @epwinchell @Hyperkid123 @martinpovolny