[RFE] Add Clone dashboard form #5730

rvsia · 2019-06-20T12:03:05Z

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1314875

Based on #5728 https://github.com/ManageIQ/manageiq/pull/18550/files

Cloud Intel > Reports > Dashboards > Select one > 'Configuration' > 'Copy selected Dashboard'

Description

Allow users to copy dashboard into the same group/different one
Uses async validation for checking name

After

yrudman · 2019-06-20T18:09:36Z

PR to keep widgets: ManageIQ/manageiq#18900

martinpovolny · 2019-06-21T14:11:45Z

We will discuss with @h-kataria if there should be a multi select for the groups.

rvsia · 2019-06-21T14:14:15Z

@h-kataria @martinpovolny

After the changes in backend copying is working well! 👍

I set initialValue of the Select group dropdown to the group of the selected dashboard - it will make a better experience for users, who just want to copy in the same group quickly
I set a validation which checks if the name is different from the original. Should I implement async validation which will check uniqueness across all dashboards? (Probably need to change the endpoint to searching by name)
So, shoud the group selection be multiselect?

h-kataria · 2019-06-21T14:54:38Z

@rvsia this looks good.
If possible we should implement async validation of uniqueness of Dashboard name within each selected group.
And yes, the groups selection should be multi-select, @yrudman was going to create a PR on the model side to accept multiple groups for cloning of dashboards.

yrudman · 2019-06-21T15:35:57Z

@h-kataria at this point dashboard name should be unique. Attempt to create dashboard with the same name for another group generates error:

yrudman · 2019-06-21T15:42:47Z

it means that multiselect will work only if we dynamically append to the supplied name something to create unique name. I do not think mutiselect in the scope of original RFE

martinpovolny · 2019-06-21T18:15:50Z

it means that multiselect will work only if we dynamically append to the supplied name something to create unique name. I do not think mutiselect in the scope of original RFE

I agree with @yrudman. Let's finish the work as it is now.

And let's ping @Loicavenel if he has comments or wants to update the requirements. If so we can do a follow-up PR and extend the copying to multiple groups once we figure what to do with the name.

rvsia · 2019-06-24T11:46:13Z

@miq-bot add_label hammer/no, react, enhancement, changelog/yes, cloud intel/dashboard, dashboards,

rvsia · 2019-06-25T12:41:05Z

@h-kataria

This button was added by the previous PR, but I don't see any way how to select any dashboard. am I right?

h-kataria · 2019-06-26T21:50:50Z

@gtanzillo @yrudman changing the scope to

validates_uniqueness_of   :name,
                              :scope => [:set_type, :owner],
                              :if    => proc { |c| c.class.in_my_region.exists?(:name => c.name) }

fixes it and seems to be working as expected

yrudman · 2019-06-26T21:57:09Z

@h-kataria yes, it is original design, when dashboards assigned to group owner_id represent group this dashboard assigned to, and group_id is nil
there are some comments explaining it: https://github.com/ManageIQ/manageiq/blob/c532321e256d57c3e427122fd4efda5420a239f8/app/models/miq_widget_set.rb#L12

yrudman · 2019-06-26T22:04:10Z

Ohh, I submitted my comment on original Harpreet's question ...

h-kataria · 2019-06-26T22:05:07Z

@yrudman when adding a new dashboard for a group, userid cannot be populated anyway so setting owner of group makes more sense. The scope should not be checking for userid i believe, changing the scope to check for set_type and owner fields will work in this case.

yrudman · 2019-06-26T22:10:34Z

I am not sure @h-kataria , for personal dashboards owner is nil and changed validation will impose the same restriction on personal dashboard name as we have now for groups - should be unique name between all user

h-kataria · 2019-06-26T22:22:05Z

@yrudman how can we populate userid when adding a new Dashboard for a group and users within the group can have a Dashboard with same name. Copy of user's dashboard is added when user logs in for the first time using same name.

h-kataria · 2019-06-27T01:11:03Z

@rvsia @gtanzillo @yrudman so i got this working by adding a line after
https://github.com/ManageIQ/manageiq-ui-classic/blob/master/app/controllers/report_controller/dashboards.rb#L74
@dashboard.group_id = g.id
Adding this lets me save a Dashboard with a unique name within a group.

Hyperkid123 · 2019-06-27T09:18:23Z

app/controllers/report_controller/dashboards.rb

@@ -51,6 +51,61 @@ def db_new
    db_edit
  end

+  def db_copy


I suppose there is no API support for copy action right?

If not, can you please create a note to the forms sheet so we don't forget to add it afterwards?

👍 Yeah, after merging this I will update the sheet!

@rvsia once we have API support for this, we can enhance this form to show the list of widgets that are part of Dashboard that's getting copied.

yrudman · 2019-06-27T11:16:21Z

@h-kataria right, saving group_id will allow to bypass existing validation.

To implement this we would need:

create data migration to adjust all existing dashboards for groups by assigning owner_id to group_id
do some analysis and possible considerable refactoring, for example, MiqWidgetSet.where_unique_on(name, user = nil) will not work as expected / designed: https://github.com/ManageIQ/manageiq/blob/1a77d38c17eb943b240db8657813db7dbcd0e6f6/app/models/miq_widget_set.rb#L25

I still think that copying dashboard to multiple groups is out of scope of original RFE

h-kataria · 2019-06-27T13:52:05Z

@h-kataria right, saving group_id will allow to bypass existing validation.

To implement this we would need:

create data migration to adjust all existing dashboards for groups by assigning owner_id to group_id

do some analysis and possible considerable refactoring, for example, MiqWidgetSet.where_unique_on(name, user = nil) will not work as expected / designed: https://github.com/ManageIQ/manageiq/blob/1a77d38c17eb943b240db8657813db7dbcd0e6f6/app/models/miq_widget_set.rb#L25

I still think that copying dashboard to multiple groups is out of scope of original RFE

@Loicavenel is it ok, to only copy a dashboard for a single Group.
@yrudman we still need to fix this so atleast the same dashboard name can be saved for a different group.

Hyperkid123 · 2019-06-27T15:50:56Z

app/javascript/components/copy-dashboard-form/copy-dashboard-form.jsx

+
+    return http.post(`/report/db_copy/${dashboardId}?button=save`, data, { skipErrors: [400] })
+      .then(() => miqAjaxButton('/report/dashboard_render'))
+      .catch(handleError, miqSparkleOff);


i don't think that you can have multiple executors in promise callbacks(unless we have some polyfill). It will only execute the first one handleError.

h-kataria · 2019-06-28T14:25:55Z

@rvsia i had a discussion with @gtanzillo fixing the Dashboard naming issue to be unique only within a group is a high risk and might introduce other issues at this time, so i am going to open a new GH issue to analyze and address the issue separately in follow up PR. For this PR we will go with single group Copy with a unique Dashboard name, the way you have in your current PR.

h-kataria · 2019-06-28T14:31:06Z

opened a GH issue in core repo ManageIQ/manageiq#18924

rvsia · 2019-07-01T14:46:59Z

@h-kataria

I changed the success message:

https://bugzilla.redhat.com/show_bug.cgi?id=1314875

miq-bot · 2019-07-02T20:28:10Z

Checked commits rvsia/manageiq-ui-classic@f34a048~...b8087c7 with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
5 files checked, 2 offenses detected

app/controllers/report_controller/dashboards.rb

❗ - Line 63, Col 7 - Style/RescueStandardError - Avoid rescuing without specifying an error class.

spec/controllers/miq_report_controller/dashboards_spec.rb

❗ - Line 16, Col 51 - Style/BracesAroundHashParameters - Redundant curly braces around a hash parameter.

terezanovotna · 2019-07-03T11:52:37Z

Hey guys,

When we copy a dashboard, I expect seeing a duplicate of previous dashboard, but the current implementation is not showing the dashboard part.

This is a screenshot of catalog I want to copy

This is a screenshot of copied catalog (which misses a dashboard section)

I expect seeing a dashboard part in copied the catalog. Or even better, when copied, take the user to an edit mode of copied catalog. These are my suggestions to improve the flow, so it is easier for the user.

terezanovotna · 2019-07-03T11:55:01Z

Adding @MariSvirik to review.

rvsia · 2019-07-03T12:36:29Z

@terezanovotna

Should the dashboard part be editable in the copy form or should it just show what is being copied? (We would need to create some new form component in React)
I agree with redirection of users to the new created (copied) dashboard (but it won't work anymore when (if) we will support copying to multiple group), however I was following instructions from BZ.

1. In the settings of dashboard, add "Copy" option. 
2. It opens up the screen where we can a)change name of the copy b)add area where we want to place the dashboard (pf3 multiselect - bootstrap version)
3. Receive a notification with what was copied. 
4. the user is taken where he started.

h-kataria · 2019-07-04T13:45:28Z

@terezanovotna it was decided to copy a dashboard and allow user to provide a name, description and a Group that the Dashboard will be copied to. If user wants to edit the Dashboard they will have to go to new Dashboard after creating a copy and then do it there. So this is working as designed.

h-kataria · 2019-07-05T14:45:13Z

@terezanovotna is this good to be merged?

terezanovotna · 2019-07-08T15:26:09Z

this can be merged, but we are not improving user experience.

The dashboard part needs to be shown in the cloned dashboard.

miq-bot added the wip label Jun 20, 2019

rvsia force-pushed the rfe-clone-dashboard branch 4 times, most recently from 0793ae7 to 29ca64a Compare June 20, 2019 12:40

h-kataria mentioned this pull request Jun 20, 2019

[WIP] - Initial changes for DDR React form to clone dashboard to other groups #5728

Closed

rvsia force-pushed the rfe-clone-dashboard branch from 29ca64a to 510e209 Compare June 21, 2019 14:24

martinpovolny self-assigned this Jun 21, 2019

rvsia force-pushed the rfe-clone-dashboard branch from e949340 to 8d9adc4 Compare June 24, 2019 11:44

rvsia changed the title ~~[WIP] RFE: Clone dashboard~~ [WIP] RFE: Add Clone dashboard form Jun 24, 2019

miq-bot added changelog/yes overview/dashboard dashboards enhancement hammer/no react labels Jun 24, 2019

rvsia force-pushed the rfe-clone-dashboard branch 4 times, most recently from fb3ad5d to 3a6bd9f Compare June 25, 2019 12:33

rvsia changed the title ~~[WIP] RFE: Add Clone dashboard form~~ [WIP] [RFE] Add Clone dashboard form Jun 25, 2019

Hyperkid123 reviewed Jun 27, 2019

View reviewed changes

h-kataria mentioned this pull request Jun 28, 2019

Dashboard names should only be unique within a group ManageIQ/manageiq#18924

Closed

rvsia force-pushed the rfe-clone-dashboard branch from 6d8df75 to 3899b3a Compare July 1, 2019 14:45

rvsia added 3 commits July 1, 2019 17:05

Add copy of dashboard form

f34a048

https://bugzilla.redhat.com/show_bug.cgi?id=1314875

Specs for Copy of dashboard form

59e693a

Add specs for dashboards clone methods

b8087c7

rvsia force-pushed the rfe-clone-dashboard branch from 3899b3a to b8087c7 Compare July 1, 2019 15:06

h-kataria added this to the Sprint 115 Ending Jul 8, 2019 milestone Jul 8, 2019

h-kataria merged commit a717862 into ManageIQ:master Jul 8, 2019

rvsia deleted the rfe-clone-dashboard branch December 10, 2019 14:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFE] Add Clone dashboard form #5730

[RFE] Add Clone dashboard form #5730

rvsia commented Jun 20, 2019 •

edited

Loading

yrudman commented Jun 20, 2019

martinpovolny commented Jun 21, 2019

rvsia commented Jun 21, 2019

h-kataria commented Jun 21, 2019

yrudman commented Jun 21, 2019

yrudman commented Jun 21, 2019

martinpovolny commented Jun 21, 2019

rvsia commented Jun 24, 2019

rvsia commented Jun 25, 2019 •

edited

Loading

h-kataria commented Jun 26, 2019 •

edited

Loading

yrudman commented Jun 26, 2019

yrudman commented Jun 26, 2019

h-kataria commented Jun 26, 2019 •

edited

Loading

yrudman commented Jun 26, 2019

h-kataria commented Jun 26, 2019

h-kataria commented Jun 27, 2019

Hyperkid123 Jun 27, 2019

rvsia Jun 27, 2019

h-kataria Jul 8, 2019

yrudman commented Jun 27, 2019

h-kataria commented Jun 27, 2019

Hyperkid123 Jun 27, 2019

h-kataria commented Jun 28, 2019

h-kataria commented Jun 28, 2019

rvsia commented Jul 1, 2019

miq-bot commented Jul 2, 2019

terezanovotna commented Jul 3, 2019

terezanovotna commented Jul 3, 2019

rvsia commented Jul 3, 2019 •

edited

Loading

h-kataria commented Jul 4, 2019

h-kataria commented Jul 5, 2019

terezanovotna commented Jul 8, 2019

[RFE] Add Clone dashboard form #5730

[RFE] Add Clone dashboard form #5730

Conversation

rvsia commented Jun 20, 2019 • edited Loading

yrudman commented Jun 20, 2019

martinpovolny commented Jun 21, 2019

rvsia commented Jun 21, 2019

h-kataria commented Jun 21, 2019

yrudman commented Jun 21, 2019

yrudman commented Jun 21, 2019

martinpovolny commented Jun 21, 2019

rvsia commented Jun 24, 2019

rvsia commented Jun 25, 2019 • edited Loading

h-kataria commented Jun 26, 2019 • edited Loading

yrudman commented Jun 26, 2019

yrudman commented Jun 26, 2019

h-kataria commented Jun 26, 2019 • edited Loading

yrudman commented Jun 26, 2019

h-kataria commented Jun 26, 2019

h-kataria commented Jun 27, 2019

Hyperkid123 Jun 27, 2019

Choose a reason for hiding this comment

rvsia Jun 27, 2019

Choose a reason for hiding this comment

h-kataria Jul 8, 2019

Choose a reason for hiding this comment

yrudman commented Jun 27, 2019

h-kataria commented Jun 27, 2019

Hyperkid123 Jun 27, 2019

Choose a reason for hiding this comment

h-kataria commented Jun 28, 2019

h-kataria commented Jun 28, 2019

rvsia commented Jul 1, 2019

miq-bot commented Jul 2, 2019

terezanovotna commented Jul 3, 2019

terezanovotna commented Jul 3, 2019

rvsia commented Jul 3, 2019 • edited Loading

h-kataria commented Jul 4, 2019

h-kataria commented Jul 5, 2019

terezanovotna commented Jul 8, 2019

rvsia commented Jun 20, 2019 •

edited

Loading

rvsia commented Jun 25, 2019 •

edited

Loading

h-kataria commented Jun 26, 2019 •

edited

Loading

h-kataria commented Jun 26, 2019 •

edited

Loading

rvsia commented Jul 3, 2019 •

edited

Loading