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

Displays a more informative message on datasource deletion. #12316

Merged
merged 2 commits into from
Nov 8, 2016
Merged

Displays a more informative message on datasource deletion. #12316

merged 2 commits into from
Nov 8, 2016

Conversation

josejulio
Copy link
Member

@mzazrivec mzazrivec self-assigned this Nov 1, 2016
@mzazrivec
Copy link
Contributor

@dclarizio Are you OK with the above message? I personally would leave the message as it is,
since it's consistent with the rest of the application. Plus the ability to delete the datasource
is controlled by rbac, which needs to be granted to a person (or well -- a role), knowing what's
going on and whether or not it's safe to delete it.

dclarizio
dclarizio reviewed Nov 2, 2016
View reviewed changes
app/helpers/application_helper/toolbar/middleware_datasources_center.rb
@@ -37,7 +37,9 @@ class ApplicationHelper::Toolbar::MiddlewareDatasourcesCenter < ApplicationHelpe
:url_parms => "main_div",
:enabled => false,
:onwhen => "1+",
:confirm => N_('Do you want to remove these datasources ?'))
:confirm => N_('Do you want to remove these datasources ? Some Applications could be using these '\
Copy link

Choose a reason for hiding this comment

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

datasources ? -> Datasources?

Should Applications be capitalized?

dclarizio
dclarizio reviewed Nov 2, 2016
View reviewed changes
app/helpers/application_helper/toolbar/middleware_datasources_center.rb
@@ -37,7 +37,9 @@ class ApplicationHelper::Toolbar::MiddlewareDatasourcesCenter < ApplicationHelpe
:url_parms => "main_div",
:enabled => false,
:onwhen => "1+",
:confirm => N_('Do you want to remove these datasources ?'))
:confirm => N_('Do you want to remove these datasources ? Some Applications could be using these '\
'datasources and might cause malfunction if are deleted.')
Copy link

Choose a reason for hiding this comment

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

Same here, capitalize Datasources

Suggesting:
might cause malfunction if are deleted -> may malfunction if they are deleted

@dclarizio
Copy link

@mzazrivec I added some editing comments to the message, but since this is for a BZ that asks to clarify the middleware datasource delete message, overall I think it's ok.

@josejulio josejulio force-pushed the ds_deletion_more_information branch from fd23a98 to 31b9eb0 Compare November 3, 2016 14:35
@josejulio
Copy link
Member Author

Updated with suggestions. Thanks!

@josejulio
Copy link
Member Author

I suppose this is euwe/yes, right?

@mzazrivec mzazrivec added this to the Sprint 49 Ending Nov 14, 2016 milestone Nov 4, 2016
@mzazrivec mzazrivec closed this Nov 4, 2016
@mzazrivec mzazrivec reopened this Nov 4, 2016
@mzazrivec mzazrivec removed this from the Sprint 49 Ending Nov 14, 2016 milestone Nov 4, 2016
@josejulio josejulio force-pushed the ds_deletion_more_information branch from 31b9eb0 to bc54547 Compare November 4, 2016 16:45
@josejulio
Copy link
Member Author

Rebased with master

@josejulio
Copy link
Member Author

Not sure why the build is failing, it doesn't seems to be related to this PR.

I tried running the tests on my local and it fails even on master branch.

@josejulio josejulio force-pushed the ds_deletion_more_information branch from bc54547 to 2c6870e Compare November 8, 2016 15:34
@pilhuhn
Copy link
Contributor

pilhuhn commented Nov 8, 2016

Another one of those
"2016-11-08 16:21:19 +0000: Rack app error: #<Sprockets::FileNotFound: couldn't find file 'es6-shim' with type 'application/javascript'" errors ... 👎

@josejulio
Copy link
Member Author

Travis is finally green 👍

jkremser
jkremser reviewed Nov 8, 2016
View reviewed changes
app/helpers/application_helper/toolbar/middleware_datasources_center.rb
@@ -37,7 +37,9 @@ class ApplicationHelper::Toolbar::MiddlewareDatasourcesCenter < ApplicationHelpe
:url_parms => "main_div",
:enabled => false,
:onwhen => "1+",
:confirm => N_('Do you want to remove these datasources ?'))
:confirm => N_('Do you want to remove these Datasources ? Some Applications could be using these '\
Copy link
Member

Choose a reason for hiding this comment

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

minor: Datasources ? -> Datasources?

Copy link
Member Author

Choose a reason for hiding this comment

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

everywhere else seems to be like that, so I left it that way.
https://github.com/josejulio/manageiq/blob/2c6870ed583e79680209d5b005790021f84d4696/app/helpers/application_helper/toolbar/middleware_deployments_center.rb#L40

Do you think I should use this PR to fix that? (I don't mind fixing it)

Copy link
Member

Choose a reason for hiding this comment

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

Yes, please, fix the typo. Then we'll merge this w/o another round of travis.

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 use [skip ci], if you make a commit just to fix the typo

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure. Is done, thanks

@miq-bot
Copy link
Member

miq-bot commented Nov 8, 2016

Checked commits josejulio/manageiq@2c6870e~...53ca7cb with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
1 file checked, 0 offenses detected
Everything looks good. ⭐

@martinpovolny martinpovolny merged commit 58261bf into ManageIQ:master Nov 8, 2016
@josejulio josejulio deleted the ds_deletion_more_information branch November 8, 2016 18:34
@chessbyte chessbyte added this to the Sprint 49 Ending Nov 14, 2016 milestone Nov 8, 2016
@chessbyte chessbyte assigned martinpovolny and unassigned mzazrivec Nov 8, 2016
simaishi pushed a commit that referenced this pull request Jan 5, 2017
@martinpovolny @simaishi
Merge pull request #12316 from josejulio/ds_deletion_more_information
e27a381 
Displays a more informative message on datasource deletion.
(cherry picked from commit 58261bf)

https://bugzilla.redhat.com/show_bug.cgi?id=1396237
@simaishi
Copy link
Contributor

simaishi commented Jan 5, 2017

Euwe backport details:

$ git log -1
commit e27a3812da169012de997306eb94f39f21b99ddb
Author: Martin Povolny <[email protected]>
Date:   Tue Nov 8 19:33:37 2016 +0100

    Merge pull request #12316 from josejulio/ds_deletion_more_information
    
    Displays a more informative message on datasource deletion.
    (cherry picked from commit 58261bfc943131157b9ae454535995c8d3e6a4c6)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1396237

@josejulio josejulio mentioned this pull request Feb 11, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dclarizio dclarizio dclarizio left review comments

@jkremser jkremser jkremser left review comments

@martinpovolny martinpovolny martinpovolny left review comments

Assignees

@martinpovolny martinpovolny

Labels
bug euwe/backported ui
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@josejulio @mzazrivec @dclarizio @pilhuhn @miq-bot @simaishi @martinpovolny @jkremser @chessbyte