Require a description when creating Snapshot #12637

borod108 · 2016-11-15T13:13:42Z

When creating a snapshot on RHV one has to write a description else
the operation will fail.
So one should not be able to start the "create snapshot" operation
without writing the description.

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

borod108 · 2016-11-15T13:32:20Z

@masayag please review.

masayag

Pls handle rubocop comment & remove the metrics_capture.rb from the PR. Besides. LGTM

masayag · 2016-11-15T15:25:41Z

app/controllers/vm_common.rb

+        add_flash(_("Description is required"), :error)
+        @in_a_form = true
+        drop_breadcrumb(:description => _("Snapshot VM '%{description}'") % {:description => @record.description}, :url => "/vm_common/snap")
+        if session[:edit] && session[:edit][:explorer]


Could you use 'dig' instead ? assuming the ci is okay with it.

Unfortunately this is not a hash its a Session object which does not support dig.

masayag · 2016-11-15T15:26:32Z

app/models/manageiq/providers/redhat/infra_manager/metrics_capture.rb

@@ -22,7 +22,6 @@ def perf_init_rhevm
    require 'ovirt_metrics'
    OvirtMetrics.establish_connection(conn_info)
  end
-


please remove this file from the PR

masayag

👍 LGTM

borod108 · 2016-11-16T13:13:10Z

@miq-bot assign @durandom

oourfali · 2016-11-16T23:54:26Z

@miq-bot add_label blocker

oourfali · 2016-11-16T23:55:05Z

@durandom please review ASAP to merge this blocker.

oourfali · 2016-11-16T23:55:54Z

@miq-bot add_label euwe/yes

durandom · 2016-11-17T07:44:44Z

app/controllers/vm_common.rb

+  def render_missing_field(session, missing_field_name)
+    add_flash(_("#{missing_field_name} is required"), :error)
+    @in_a_form = true
+    drop_breadcrumb(:name => _("Snapshot VM '#{@record.name}'"), :url => "/vm_common/snap")


this won't work with i18n.
please use the format in the original code
See http://manageiq.org/docs/guides/i18n and search for 'string interpolation'

Oh I had no idea. Tnx!

durandom

besides i18n stuff good

durandom · 2016-11-17T07:45:31Z

app/controllers/vm_common.rb

@@ -558,6 +558,18 @@ def snap
  end
  alias_method :vm_snapshot_add, :snap

+  def render_missing_field(session, missing_field_name)
+    add_flash(_("#{missing_field_name} is required"), :error)


same i18n thing here

durandom · 2016-11-17T07:46:17Z

app/controllers/vm_common.rb

@@ -573,15 +585,9 @@ def snap_vm
      @name = params[:name]
      @description = params[:description]
      if params[:name].blank? && [email protected](:snapshot_name_optional?)
-        add_flash(_("Name is required"), :error)


+1 for refactoring into a method

tnx. I just discovered I have a Vim plugin to extract methods ))
Although it did put the method inside a non related if block at first, but it was still nice :)

yep, I use IntelliJ with vim bindings and like the refactoring tools there as well

durandom · 2016-11-17T07:48:03Z

app/controllers/vm_common.rb

-          render :action => "snap"
-        end
+        render_missing_field(session, "Name")
+      elsif params[:description].blank? && @record.try(:snapshot_description_required?)


minor, but I always prefer try! over try, cause the former one will raise an exception if you call it on nil.

It seems to be the opposite of what i want here,
http://api.rubyonrails.org/classes/Object.html#method-i-try-21
"try! - Same as try, but raises a NoMethodError exception if the receiver is not nil and does not implement the tried method."
All other providers do not currently implement this method so I just want it to return nil rather then throw an exception.

oh, you are absolutely right! so scratch that. It was just a reflex 😄

When creating a snapshot on RHV one has to write a description else the operation will fail. So one should not be able to start the "create snapshot" operation without writing the description. https://bugzilla.redhat.com/show_bug.cgi?id=1384517

miq-bot · 2016-11-17T09:13:24Z

Checked commit borod108@06c8bc3 with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
2 files checked, 0 offenses detected
Everything looks good. 🍪

borod108 · 2016-11-17T09:58:06Z

@durandom there is a strange error from CI - https://travis-ci.org/ManageIQ/manageiq/jobs/176640356
I will close and reopen the PR.
If this error is nothing to worry about can it please be merged?

durandom

from me. Let's wait until CI works again

durandom · 2016-11-17T15:01:55Z

@miq-bot assign @agrare
@agrare please merge

agrare · 2016-11-17T15:05:30Z

Nice solution @borod108 👍

chessbyte · 2016-11-17T19:45:51Z

Euwe Backport conflict:

$ git cherry-pick -e -x -m 1 d643265 
error: could not apply d643265... Merge pull request #12637 from borod108/bugs/1384517snapshot_desc_req
hint: after resolving the conflicts, mark the corrected paths
hint: with 'git add <paths>' or 'git rm <paths>'
hint: and commit the result with 'git commit'

$ git status
On branch euwe
Your branch is up-to-date with 'upstream/euwe'.
You are currently cherry-picking commit d643265.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:

    modified:   app/models/manageiq/providers/redhat/infra_manager/vm/operations/snapshot.rb

Unmerged paths:
  (use "git add <file>..." to mark resolution)

    both modified:   app/controllers/vm_common.rb

$ git diff

diff --cc app/controllers/vm_common.rb
index 8a684de,f05fe2a..0000000
--- a/app/controllers/vm_common.rb
+++ b/app/controllers/vm_common.rb
@@@ -585,15 -587,9 +599,21 @@@ module VmCommo
        @name = params[:name]
        @description = params[:description]
        if params[:name].blank? && [email protected](:snapshot_name_optional?)
++<<<<<<< HEAD
 +        add_flash(_("Name is required"), :error)
 +        @in_a_form = true
 +        drop_breadcrumb(:name => _("Snapshot VM '%{name}'") % {:name => @record.name}, :url => "/vm_common/snap")
 +        if session[:edit] && session[:edit][:explorer]
 +          @edit = session[:edit]    # saving it to use in next transaction
 +          render :partial => "shared/ajax/flash_msg_replace"
 +        else
 +          render :action => "snap"
 +        end
++=======
+         render_missing_field(session, "Name")
+       elsif params[:description].blank? && @record.try(:snapshot_description_required?)
+         render_missing_field(session, "Description")
++>>>>>>> d643265... Merge pull request #12637 from borod108/bugs/1384517snapshot_desc_req
        else
          flash_error = false
          #       audit = {:event=>"vm_genealogy_change", :target_id=>@record.id, :target_class=>@record.class.base_class.name, :userid => session[:userid]}

…desc_req Require a description when creating Snapshot (cherry picked from commit d643265)

borod108 · 2016-11-18T12:26:24Z

Conflict resolution: #12742

chessbyte · 2016-11-20T13:33:44Z

Backported to Euwe via #12742

chessbyte · 2016-11-20T13:34:33Z

Euwe BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1396207

borod108 force-pushed the bugs/1384517snapshot_desc_req branch from aae2270 to 41dd7b8 Compare November 15, 2016 14:59

masayag reviewed Nov 15, 2016

View reviewed changes

borod108 force-pushed the bugs/1384517snapshot_desc_req branch from 41dd7b8 to 0fd3268 Compare November 16, 2016 09:22

masayag approved these changes Nov 16, 2016

View reviewed changes

borod108 force-pushed the bugs/1384517snapshot_desc_req branch 2 times, most recently from 30be3b0 to cffd14b Compare November 16, 2016 10:45

miq-bot assigned durandom Nov 16, 2016

miq-bot added the blocker label Nov 16, 2016

miq-bot added the euwe/yes label Nov 16, 2016

durandom reviewed Nov 17, 2016

View reviewed changes

durandom suggested changes Nov 17, 2016

View reviewed changes

borod108 force-pushed the bugs/1384517snapshot_desc_req branch from cffd14b to 06c8bc3 Compare November 17, 2016 09:10

borod108 closed this Nov 17, 2016

borod108 reopened this Nov 17, 2016

durandom approved these changes Nov 17, 2016

View reviewed changes

borod108 closed this Nov 17, 2016

borod108 reopened this Nov 17, 2016

miq-bot assigned agrare and unassigned durandom Nov 17, 2016

agrare added the providers/rhevm label Nov 17, 2016

agrare merged commit d643265 into ManageIQ:master Nov 17, 2016

agrare added this to the Sprint 50 Ending Dec 5, 2016 milestone Nov 17, 2016

agrare added the bug label Nov 17, 2016

chessbyte added the euwe/conflict label Nov 17, 2016

borod108 pushed a commit to borod108/manageiq that referenced this pull request Nov 18, 2016

Merge pull request ManageIQ#12637 from borod108/bugs/1384517snapshot_…

db54a22

…desc_req Require a description when creating Snapshot (cherry picked from commit d643265)

borod108 mentioned this pull request Nov 18, 2016

[EUWE] Require a description when creating Snapshot #12742

Merged

chessbyte added euwe/backported and removed euwe/conflict labels Nov 20, 2016

Fryguy added the providers/infrastructure label Jan 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Require a description when creating Snapshot #12637

Require a description when creating Snapshot #12637

borod108 commented Nov 15, 2016 •

edited by miq-bot

Loading

borod108 commented Nov 15, 2016

masayag left a comment

masayag Nov 15, 2016

borod108 Nov 16, 2016

masayag Nov 15, 2016

borod108 Nov 16, 2016

masayag left a comment

borod108 commented Nov 16, 2016

oourfali commented Nov 16, 2016

oourfali commented Nov 16, 2016

oourfali commented Nov 16, 2016

durandom Nov 17, 2016

borod108 Nov 17, 2016

durandom left a comment

durandom Nov 17, 2016

durandom Nov 17, 2016

borod108 Nov 17, 2016

durandom Nov 17, 2016

durandom Nov 17, 2016

borod108 Nov 17, 2016

durandom Nov 17, 2016

miq-bot commented Nov 17, 2016

borod108 commented Nov 17, 2016

durandom left a comment

durandom commented Nov 17, 2016

agrare commented Nov 17, 2016

chessbyte commented Nov 17, 2016

borod108 commented Nov 18, 2016

chessbyte commented Nov 20, 2016

chessbyte commented Nov 20, 2016

Require a description when creating Snapshot #12637

Require a description when creating Snapshot #12637

Conversation

borod108 commented Nov 15, 2016 • edited by miq-bot Loading

borod108 commented Nov 15, 2016

masayag left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

masayag left a comment

Choose a reason for hiding this comment

borod108 commented Nov 16, 2016

oourfali commented Nov 16, 2016

oourfali commented Nov 16, 2016

oourfali commented Nov 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

durandom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented Nov 17, 2016

borod108 commented Nov 17, 2016

durandom left a comment

Choose a reason for hiding this comment

durandom commented Nov 17, 2016

agrare commented Nov 17, 2016

chessbyte commented Nov 17, 2016

borod108 commented Nov 18, 2016

chessbyte commented Nov 20, 2016

chessbyte commented Nov 20, 2016

borod108 commented Nov 15, 2016 •

edited by miq-bot

Loading