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

Timeprofile copy fix #3835

Merged
merged 3 commits into from
May 10, 2018
Merged

Timeprofile copy fix #3835

merged 3 commits into from
May 10, 2018

Conversation

ZitaNemeckova
Copy link
Contributor

@ZitaNemeckova ZitaNemeckova commented Apr 24, 2018 •
edited
Loading

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

Introduced in ManageIQ/manageiq#10524

How to reproduce:
My Settings -> Time Profiles -> select one time profile -> Configuration -> Copy
Before:
Form is empty. Nothing pre-filled.
After:
Form is pre-filled.

@miq-bot add_label wip, bug, angular dialogs, gaprindashvili/yes, fine/yes, euwe/yes

@miq-bot miq-bot changed the title Timeprofile copy fixes [WIP] Timeprofile copy fixes Apr 24, 2018
@ZitaNemeckova
Specs fixes
cb132d5 
Add timeProfileFormAction to specs
Fix specs descriptions so they no longer lie
Add spec for timeprofile_copy
ZitaNemeckova
ZitaNemeckova commented Apr 24, 2018
View reviewed changes
app/views/configuration/_timeprofile_form.html.haml Outdated
@@ -224,4 +224,5 @@

:javascript
ManageIQ.angular.app.value('timeProfileFormId', '#{@timeprofile.id || "new"}');
ManageIQ.angular.app.value('timeProfileFormAction', '#{params[:action]}');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cannot be passed directly.

@ZitaNemeckova ZitaNemeckova reopened this Apr 26, 2018
@ZitaNemeckova ZitaNemeckova changed the title [WIP] Timeprofile copy fixes [WIP] Timeprofile copy fix Apr 26, 2018
@ZitaNemeckova ZitaNemeckova reopened this Apr 27, 2018
@ZitaNemeckova ZitaNemeckova reopened this May 2, 2018
@miq-bot
Copy link
Member

miq-bot commented May 2, 2018

Checked commits ZitaNemeckova/manageiq-ui-classic@a1323d4~...39913cf with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 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.

@ZitaNemeckova
Copy link
Contributor Author

@miq-bot remove_label wip

@hstastna can you please review, thanks :)

@miq-bot miq-bot changed the title [WIP] Timeprofile copy fix Timeprofile copy fix May 2, 2018
@miq-bot miq-bot removed the wip label May 2, 2018
hstastna
hstastna reviewed May 4, 2018
View reviewed changes
app/controllers/configuration_controller.rb
@@ -253,6 +254,7 @@ def timeprofile_new
def timeprofile_edit
assert_privileges("tp_edit")
@timeprofile = TimeProfile.find(params[:id])
@timeprofile_action = "timeprofile_edit"
Copy link

@hstastna hstastna May 4, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@ZitaNemeckova The code looks good and it works. I just haven't checked the specs. Anyway, I wonder why we need https://github.com/ManageIQ/manageiq-ui-classic/pull/3835/files#diff-679158917bc83d666b7fa919b44817ebR246
and https://github.com/ManageIQ/manageiq-ui-classic/pull/3835/files#diff-679158917bc83d666b7fa919b44817ebR257 if this is already set in params[;action] - maybe we could use it instead of setting @timeprofile_action.. but maybe not :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's preventing Cross-Site Scripting (Hakiri is catching this kind of vulnerabilities). I cannot pass anything from params directly to haml.

Copy link

Choose a reason for hiding this comment

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

Ah, of course! Forgot.. Thanks! 👍

@hstastna
Copy link

hstastna commented May 4, 2018 •
edited
Loading

@ZitaNemeckova Copying works well but I just found another thing: after I clicked on Copy selected Time Profile and made some changes but did not change the Description and clicked on Save button, this has happened:
new2_same_name
I was even able to create more Time Profiles with the same name by copying the existing one, not just two of them. Is this behavior ok or is it a bug?

@ZitaNemeckova
Copy link
Contributor Author

@hstastna I'm not sure about description. There was no check that description is unique but it seems user unfriendly.

@mzazrivec mzazrivec self-assigned this May 10, 2018
@mzazrivec mzazrivec added this to the Sprint 86 Ending May 21, 2018 milestone May 10, 2018
@mzazrivec mzazrivec merged commit 4193c68 into ManageIQ:master May 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hstastna hstastna hstastna left review comments

Assignees

@mzazrivec mzazrivec

Labels
angular dialogs bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ZitaNemeckova @miq-bot @hstastna @mzazrivec