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

Replace provision Tagging tree by component #5550

Merged
merged 13 commits into from
Aug 5, 2019
Merged

Replace provision Tagging tree by component #5550

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
f7abada
Replace Provision Tags tree with React component
PanSpagetka May 9, 2019
4ec58a9
Fix specs for Provision taggiing middleware
PanSpagetka Jun 11, 2019
5b16984
Remove miqOnCheckProvTags function
PanSpagetka Jun 11, 2019
1ad1df0
Code styling
PanSpagetka Jun 11, 2019
6aaf393
Change tagging provision markup to be col-md-12 in total
PanSpagetka Jul 25, 2019
92b98fe
Update midleware specs for tagging
PanSpagetka Jul 25, 2019
b60de71
Remove unnecessary tagging info text
PanSpagetka Jul 29, 2019
8f3109d
Fix single tag selection for provision
PanSpagetka Jul 29, 2019
a410aac
Use new version of react-ui-components
PanSpagetka Jul 29, 2019
c9528dc
Change tags PropType to string or number
PanSpagetka Jul 30, 2019
bf430f2
Remove all tags tree
PanSpagetka Jul 30, 2019
1bd910a
Fix delete for provision tagging
PanSpagetka Jul 31, 2019
2ce52a3
Fix tagging middleware specs
PanSpagetka Aug 5, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 0 additions & 20 deletions app/assets/javascripts/miq_tree.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,25 +120,6 @@ function miqExpandParentNodes(treename, selected_node) {
}
}

// OnCheck handler for the tags trees
function miqOnCheckProvTags(node, treename) {
var tree = miqTreeObject(treename);
// Allow only one node among siblings to be checked
if (node.state.checked) {
var siblings = $.grep(tree.getParents(node)[0].nodes, function(item) {
return item.key !== node.key;
});
tree.uncheckNode(siblings, {silent: true });
}

var all_checked = tree.getChecked().map(function(item) {
return encodeURIComponent(item.key);
});

miqJqueryRequest(ManageIQ.tree.checkUrl + '?ids_checked=' + all_checked);
return true;
}

function miqOnClickSelectRbacTreeNode(id) {
var tree = 'rbac_tree';
miqTreeExpandNode(tree, 'xx-' + id.split('-')[0]);
Expand Down Expand Up @@ -433,7 +414,6 @@ function miqTreeEventSafeEval(func) {
'miqOnCheckGeneric',
'miqOnClickMenuRoles',
'miqOnCheckProtect',
'miqOnCheckProvTags',
'miqOnCheckSections',
'miqOnCheckUserFilters',
'miqOnClickAutomate',
Expand Down
93 changes: 30 additions & 63 deletions app/controllers/application_controller/miq_request_methods.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -651,7 +651,7 @@ def prov_req_submit
# Get variables from provisioning form
def prov_get_form_vars
if params[:ids_checked] # User checked/unchecked a tree node
ids = params[:ids_checked].split(",")
ids = params[:ids_checked]
# for some reason if tree is not expanded clicking on radiobuttons this.getAllChecked() sends up extra blanks
@edit.store_path(:new, tag_symbol_for_workflow, ids.select(&:present?).collect(&:to_i))
end
Expand Down Expand Up @@ -984,69 +984,36 @@ def workflow_instance_from_vars(req)
end

def build_tags_tree(wf, vm_tags, edit_mode)
tags = wf.send("allowed_tags")
@curr_tag = nil
# Build the default filters tree for the search views
all_tags = [] # Array to hold all CIs
kids_checked = false
tags.each_with_index do |t, i| # Go thru all of the Searches
if @curr_tag.blank? || @curr_tag != t[:name]
if @curr_tag != t[:name] && @ci_node
@ci_node[:expand] = true if kids_checked
kids_checked = false
@ci_node[:children] = @ci_kids if @ci_kids.present?
all_tags.push(@ci_node) if @ci_kids.present?
end
@curr_tag = t[:name]
@ci_node = {} # Build the ci node
@ci_node[:key] = t[:id].to_s
@ci_node[:title] = t[:description]
@ci_node[:title] += " *" if t[:single_value]
@ci_node[:tooltip] = t[:description]
@ci_node[:addClass] = "cfme-no-cursor-node" # No cursor pointer
@ci_node[:icon] = 'pficon pficon-folder-close'
@ci_node[:hideCheckbox] = @ci_node[:cfmeNoClick] = true
@ci_node[:addClass] = "cfme-bold-node" # Show node as different
@ci_kids = []
end
if @curr_tag.present? && @curr_tag == t[:name]
t[:children].each do |c|
temp = {}
temp[:key] = c[0].to_s
# only add cfme_parent_key for single value tags, need to use in JS onclick handler
temp[:selectable] = false
temp[:title] = temp[:tooltip] = c[1][:description]
temp[:addClass] = "cfme-no-cursor-node"
temp[:icon] = 'fa fa-tag'
if edit_mode # Don't show checkboxes/radio buttons in non-edit mode
if vm_tags && vm_tags.include?(c[0].to_i)
temp[:select] = true
kids_checked = true
else
temp[:select] = false
end
if @edit && @edit[:current][tag_symbol_for_workflow] != @edit[:new][tag_symbol_for_workflow]
# checking to see if id is in current but not in new, change them to blue OR if id is in current but deleted from new
if (!@edit[:current][tag_symbol_for_workflow].include?(c[0].to_i) && @edit[:new][tag_symbol_for_workflow].include?(c[0].to_i)) ||
(!@edit[:new][tag_symbol_for_workflow].include?(c[0].to_i) && @edit[:current][tag_symbol_for_workflow].include?(c[0].to_i))
temp[:addClass] = "cfme-blue-bold-node"
end
end
@ci_kids.push(temp) unless @ci_kids.include?(temp)
else
temp[:hideCheckbox] = true
@ci_kids.push(temp) unless @ci_kids.include?(temp) || !vm_tags.include?(c[0].to_i)
end
end
end
if i == tags.length - 1 # Adding last node
@ci_node[:expand] = true if kids_checked
kids_checked = false
@ci_node[:children] = @ci_kids if @ci_kids.present?
all_tags.push(@ci_node) if @ci_kids.present?
end
# for some reason @tags is set in wf, and it is changed by map bellow which causes bugs
wf.instance_variable_set(:@tags, nil)
Copy link
Member

Choose a reason for hiding this comment

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

What's wf? If it is undocumented behavior, please look it up and document it.

Copy link
Contributor

Choose a reason for hiding this comment

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

stands for workflow, used a lot in automate code (309 occurences)

tags = wf.allowed_tags.map do |cat|
{
:values => cat[:children].map do |tag|
{:id => tag.first, :description => tag.second[:description]}
end,
:id => cat[:name],
:description => cat[:description],
:singleValue => cat[:single_value],
}
end
@all_tags_tree = TreeBuilder.convert_bs_tree(all_tags).to_json # Add ci node array to root of tree

assignments = Classification.find(vm_tags)
assigned_tags = assignments.map do |tag|
{
:description => tag.parent.description,
:id => tag.parent.name,
:singleValue => tag.parent.single_value,
:values => ->(arr, single_value) { single_value ? [arr.last] : arr }.call(
assignments.select do |assignment|
assignment.parent.name == tag.parent.name
end,
tag.parent.single_value
).map do |assignment|
{ :description => assignment.description, :id => assignment.id }
end
}
end.uniq
@tags = {:tags => tags, :assignedTags => assigned_tags, :affectedItems => []}
end

def build_template_filter
Expand Down
1 change: 0 additions & 1 deletion app/controllers/vm_common.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1193,7 +1193,6 @@ def replace_right_cell(options = {})
locals[:submit_button] = @sb[:action] != 'miq_request_new' # need submit button on the screen
locals[:continue_button] = @sb[:action] == 'miq_request_new' # need continue button on the screen
update_buttons(locals) if @edit && @edit[:buttons].present?
presenter[:clear_tree_cookies] = "all_tags_tree"
end

if ['snapshot_add'].include?(@sb[:action])
Expand Down
34 changes: 27 additions & 7 deletions app/javascript/components/taggingWrapper.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,26 @@ import { Spinner } from 'patternfly-react';
import { TaggingWithButtonsConnected, TaggingConnected, taggingApp } from '@manageiq/react-ui-components/dist/tagging';
import { http } from '../http_api';

const params = (type = 'default', state, tag = {}) => ({
provision: {
id: "new",
ids_checked: [state.tagging.appState.assignedTags.map(t => t.values.map(val => val.id)).flat(), tag.tagValue.id].flat(),
tree_typ: 'tags'
},
default: {
id: state.tagging.appState.affectedItems[0] || "new",
cat: tag.tagCategory.id,
val: tag.tagValue.id,
check: 1,
tree_typ: 'tags'
}
})[type]

const onDelete = (type = 'default', params = [], deleted_element) => ({
provision: {...params, check: 0, ids_checked: params.ids_checked.filter(element => element !== deleted_element) },
default: params,
})[type]

class TaggingWrapper extends React.Component {
constructor(props) {
super(props);
Expand All @@ -22,7 +42,7 @@ class TaggingWrapper extends React.Component {
render() {
if (!this.props.isLoaded) return <Spinner loading size="lg" />;
const { urls, options, tagging } = this.props;
return (options && options.hideButtons && <TaggingConnected options={{...options}}/> || <TaggingWithButtonsConnected
return (options && options.hideButtons && <TaggingConnected options={{...options, params, onDelete }}/> || <TaggingWithButtonsConnected
saveButton={{
// FIXME: jQuery is necessary here as it communicates with the old world
// don't replace $.post with http.post
Expand Down Expand Up @@ -53,7 +73,7 @@ class TaggingWrapper extends React.Component {
description: __('Reset'),
}
}
options={{...options}}
options={{...options, params, onDelete }}
/>);
}
}
Expand All @@ -65,21 +85,21 @@ TaggingWrapper.propTypes = {
urls: PropTypes.shape({
cancel_url: PropTypes.string.isRequired,
save_url: PropTypes.string.isRequired,
}).isRequired,
}),
tags: PropTypes.shape({
tags: PropTypes.arrayOf(PropTypes.shape({
id: PropTypes.number.isRequired,
id: PropTypes.oneOfType([PropTypes.string, PropTypes.number]).isRequired,
description: PropTypes.string.isRequired,
values: PropTypes.arrayOf(PropTypes.shape({
id: PropTypes.number.isRequired,
id: PropTypes.oneOfType([PropTypes.string, PropTypes.number]).isRequired,
description: PropTypes.string.isRequired,
}).isRequired).isRequired,
})).isRequired,
assignedTags: PropTypes.arrayOf(PropTypes.shape({
id: PropTypes.number.isRequired,
id: PropTypes.oneOfType([PropTypes.string, PropTypes.number]).isRequired,
description: PropTypes.string.isRequired,
values: PropTypes.arrayOf(PropTypes.shape({
id: PropTypes.number.isRequired,
id: PropTypes.oneOfType([PropTypes.string, PropTypes.number]).isRequired,
description: PropTypes.string.isRequired,
}).isRequired).isRequired,
})).isRequired,
Expand Down
7 changes: 4 additions & 3 deletions app/javascript/miq-redux/middleware.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,12 @@ import promiseMiddleware from 'redux-promise-middleware';
export const taggingMiddleware = store => next => action => {
const { type, meta, tagCategory, tag } = action;
if (meta && meta.url) {
Copy link
Contributor

@himdel himdel Jun 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

switch (type) {
  case 'UI-COMPONENTS_TAGGING_TOGGLE_TAG_VALUE_CHANGE':
    $.post(meta.url, meta.params(meta.type, store.getState(), tag));
    break;
  case 'UI-COMPONENTS_TAGGING_DELETE_ASSIGNED_TAG':
    $.post(meta.url, meta.onDelete(meta.type, params, tag.tagValue.id));
    break;
}

and move that check: 0 to onDelete definition?

Copy link
Contributor

@himdel himdel Jun 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Or ideally call the same method, since you're passing the type anyway. (type != meta.type, sorry)

The point is, there should be only 1 place dealing with this logic, not 2, or 3.

(maybe onDelete should not take computed params, and do it by itself, from state, that way maybe you can also unify the args)

const params = {id: store.getState().tagging.appState.affectedItems[0], cat: tag.tagCategory.id, val: tag.tagValue.id, check: 1, tree_typ: 'tags' };
let params = meta.params(meta.type, store.getState(), tag);
if (type === 'UI-COMPONENTS_TAGGING_TOGGLE_TAG_VALUE_CHANGE') {
$.post(meta.url, params)
$.post({url: meta.url, data: JSON.stringify(params), contentType: "application/json"})
} else if (type === 'UI-COMPONENTS_TAGGING_DELETE_ASSIGNED_TAG') {
$.post(meta.url, {...params, check: 0})
params = meta.onDelete(meta.type, params, tag.tagValue.id);
$.post({url: meta.url, data: JSON.stringify(params), contentType: "application/json"})
}
}
let result = next(action)
Expand Down
31 changes: 24 additions & 7 deletions app/javascript/spec/miq-redux/middleware.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,22 @@ import { taggingMiddleware } from '../../miq-redux/middleware'; //this is your m
// const next = jest.fn(); // middleware needs those as parameters, usually calling next(action) at the end to proceed
// const store = jest.fn();

const params = (type = 'default', state, tag = {}) => ({
provision: {
id: "new",
ids_checked: [],
tree_typ: 'tags'
},
default: {
id: [],
cat: tag.tagCategory.id,
val: tag.tagValue.id,
check: 1,
tree_typ: 'tags'
}
})[type]

const onDelete = (x, y, z) => ({...y, check: 0});

it('passes the intercepted action to next', () => {

Expand All @@ -23,15 +39,16 @@ it('calls post for UI-COMPONENTS_TAGGING_TOGGLE_TAG_VALUE_CHANGE action', () =>
getState: () => ({
tagging: {
appState: {
affectedItems: [{}]
affectedItems: [{}],
assignedTags: []
}
}
})
};
const action = { type: 'UI-COMPONENTS_TAGGING_TOGGLE_TAG_VALUE_CHANGE', meta: { url: 'url/bla' }, tag: {tagCategory: {id: 1}, tagValue: { id:2 }}}
const action = { type: 'UI-COMPONENTS_TAGGING_TOGGLE_TAG_VALUE_CHANGE', meta: { url: 'url/bla', params: params }, tag: {tagCategory: {id: 1}, tagValue: { id:2 }}}
taggingMiddleware(store)(next)(action);
expect(next.mock.calls).toEqual( [[{"meta": {"url": "url/bla"}, "tag": {"tagCategory": {"id": 1}, "tagValue": {"id": 2}}, "type": "UI-COMPONENTS_TAGGING_TOGGLE_TAG_VALUE_CHANGE"}]]);
expect(spy).toHaveBeenCalledWith("url/bla", {"cat": 1, "check": 1, "id": {}, "tree_typ": "tags", "val": 2});
expect(next.mock.calls).toEqual( [[{"meta": {"url": "url/bla", "params": params}, "tag": {"tagCategory": {"id": 1}, "tagValue": {"id": 2}}, "type": "UI-COMPONENTS_TAGGING_TOGGLE_TAG_VALUE_CHANGE"}]]);
expect(spy).toHaveBeenCalledWith({"contentType": "application/json", "data": "{\"id\":[],\"cat\":1,\"val\":2,\"check\":1,\"tree_typ\":\"tags\"}", "url": "url/bla"});
});


Expand All @@ -48,8 +65,8 @@ it('calls post for UI-COMPONENTS_TAGGING_DELETE_ASSIGNED_TAG action', () => {
}
})
};
const action = { type: 'UI-COMPONENTS_TAGGING_DELETE_ASSIGNED_TAG', meta: { url: 'url/bla' }, tag: {tagCategory: {id: 1}, tagValue: { id:2 }}};
const action = { type: 'UI-COMPONENTS_TAGGING_DELETE_ASSIGNED_TAG', meta: { url: 'url/bla', params: params, onDelete: onDelete, type: 'default' }, tag: {tagCategory: {id: 1}, tagValue: { id:2 }}};
taggingMiddleware(store)(next)(action);
expect(next.mock.calls).toEqual( [[{"meta": {"url": "url/bla"}, "tag": {"tagCategory": {"id": 1}, "tagValue": {"id": 2}}, "type": 'UI-COMPONENTS_TAGGING_DELETE_ASSIGNED_TAG'}]]);
expect(spy).toHaveBeenCalledWith("url/bla", {"cat": 1, "check": 0, "id": {}, "tree_typ": "tags", "val": 2});
expect(next.mock.calls).toEqual( [[{"meta": {"url": "url/bla", params: params, onDelete: onDelete, type: 'default'}, "tag": {"tagCategory": {"id": 1}, "tagValue": {"id": 2}}, "type": 'UI-COMPONENTS_TAGGING_DELETE_ASSIGNED_TAG'}]]);
expect(spy).toHaveBeenCalledWith({"contentType": "application/json", "data": "{\"id\":[],\"cat\":1,\"val\":2,\"check\":0,\"tree_typ\":\"tags\"}", "url": "url/bla"});
});
2 changes: 0 additions & 2 deletions app/views/miq_request/_prov_edit.html.haml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,3 @@
- unless @edit[:explorer]
= render :partial => 'miq_request/prov_form_buttons'
= _("Note: Fields marked with * are required.")
:javascript
miqDeleteTreeCookies('all_tags_tree')
22 changes: 4 additions & 18 deletions app/views/miq_request/_prov_field.html.haml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
- return if field_hash.blank?
- field_id = dialog.to_s + "__" + field.to_s
- disabled ||= false

- unless [:hide, :ignore].include?(field_hash[:display])
:javascript
// Create from/to date JS vars to limit calendar starting from
Expand Down Expand Up @@ -290,26 +289,13 @@
= field_hash[:notes]
- elsif [:tag_ids, :vm_tags].include?(field)
-# tree control for tags fields
.col-md-8
#all_tags_treebox.treeview-pf-hover.treeview-pf-select{:style => "color:#000; overflow: hidden;"}
- if @edit && @edit[:req_id]
- check = @edit[:req_id]
- elsif @miq_request
- check = @miq_request.id
- else
- check = 'new'
= render(:partial => "layouts/tree",
:locals => {:tree_id => "all_tags_treebox",
:tree_name => "all_tags_tree",
:bs_tree => @all_tags_tree,
:oncheck => "miqOnCheckProvTags",
Copy link
Contributor

Choose a reason for hiding this comment

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

also remove the miqOnCheckProvTags function please (and from the whitelist)?

(If it was used miq_prov_vars would still need the split .. but it is dead now :).)

:check_url => "/miq_request/prov_field_changed/#{check}",
:checkboxes => true})
.col-md-10
= react('TaggingWrapperConnected',
:tags => @tags,
:options => { :type => 'provision', :hideHeaders => true, :hideButtons => true, :url => url_for_only_path(:action => 'prov_field_changed')})
- unless field_hash[:notes_display] == :hide || field_hash[:notes].blank?
-# Display notes if available
= field_hash[:notes]
.note
= _("* Only a single value can be assigned from these Tag Categories")
- elsif [:attached_ds, :iso_image_id, :placement_availability_zone,
:placement_cluster_name, :placement_dc_name, :placement_ds_name,
:placement_ems_name, :placement_host_name, :placement_rp_name,
Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
"dependencies": {
"@data-driven-forms/pf3-component-mapper": "^1.13.3",
"@data-driven-forms/react-form-renderer": "^1.13.3",
"@manageiq/react-ui-components": "~0.11.27",
"@manageiq/react-ui-components": "~0.11.35",
"@manageiq/ui-components": "~1.2.10",
"@pf3/select": "~1.12.6",
"@pf3/timeline": "~1.0.8",
Expand Down