Service retirement values from dialog #16799

gmcculloug · 2018-01-10T22:55:15Z

Building on the changes from PR #16338 (Name service during provisioning from dialog input) this PR adds the ability to set service retirement values from a dialog during service ordering.

The logic sets the retires_on value based on the dialog field name used:

service_retires_on - Date/DateTime value
service_retires_in_days - Duration value (Integer) from current time.
service_retires_in_hours - Duration value (Integer) from current time.

The retirement_warn value for a service is set only if a retires_on value has been specified. It can be set using one of the following dialog field names:

service_retirement_warn_on - Date/DataTime value
service_retirement_warn_in_days - Duration value (Integer) from current time.
service_retirement_warn_in_hours - Duration value (Integer) from current time.
service_retirement_warn_offset_days - Duration value (Integer) from retries_on value
service_retirement_warn_offset_hours - Duration value (Integer) from retries_on value

Links

Steps for Testing/QA

Create a service dialog that specifics one of the fields that sets the retires_on value for a service. Optionally, set a dialog field to specify the retirement_warn value. After ordering the Catalog Item the Service that is created should have the expected retirement properties.

gmcculloug · 2018-01-10T22:56:48Z

Tests in progress as well as code changes to protect against parsing invalid values.

miq-bot · 2018-01-31T22:23:27Z

This pull request is not mergeable. Please rebase and repush.

gmcculloug · 2018-07-30T17:40:09Z

@mkanoor @tinaafitz @eclarizio Still WIP as I need to a several more retirement tests, but I have changed the structure of the code and would appreciate comments/suggestions.

JPrause · 2018-08-07T13:31:55Z

@miq-bot add_label blocker

JPrause · 2018-08-07T13:32:50Z

@gmcculloug if this can be backported to fine and gaprindashvili can you add: fine/yes and gaprindashvili/yes

JPrause · 2018-08-20T14:58:46Z

@gmcculloug is this still a WIP. We have a build today, and wondering if it's ready, so it can be backported.

gmcculloug · 2018-08-20T18:30:05Z

Yes, still wip so not ready for the build.

eclarizio

Overall I think this looks like a good step but I'm extremely concerned about how all of the various attributes are set via the meta-programming. I think we could break some of the more complicated attribute setting logic into another class so that it's easier to test, and also easier to see the explicit call instead of just a send.

Ultimately your #parse_options method I think would change to something like:

@attributes[:name] = @options["dialog_service_name"] if @options["dialog_service_name"]
@attributes[:description] = @options["dialog_service_description"] if @options["dialog_service_description"]
retirement_options = SomeRetirementOptionParser.do_the_retirement_parsing(@options)
@attributes.merge(retirement_options)

eclarizio · 2018-08-23T21:43:18Z

app/models/service/dialog_properties.rb

+    end
+
+    def parse_value(field_name, dialog_field_name, dialog_field_value)
+      send(field_name, dialog_field_value) if @options.key?(dialog_field_name)


Do we need this if? We're already qualifying the same thing on line 26, and I don't see anywhere else this method is used.

Also I'm concerned about this method since it's a pretty huge feat of meta-programming (hats off to you for accomplishing that one) and it's going to be extremely difficult to unravel later down the line if something needs changing here. Not sure how to solve it quite yet, but maybe some of my other suggestions will make this easier to digest.

eclarizio · 2018-08-23T21:50:04Z

app/models/service/dialog_properties.rb

+    end
+
+    def retires_on(value)
+      self.retire_on_date = time_parse(value)


At first I was like "wait a second, why aren't we just doing @attributes[:retire_on_date] = time_parse(value) but then I realized you overrode retire_on_date=. I feel like this whole "retirement" part of this code could be broken out into a whole separate class that purely deals with the parsing of the time. The warning part could even be it's own separate class too, I think.

eclarizio · 2018-08-23T21:51:49Z

spec/models/service_template_spec.rb

@@ -1,6 +1,9 @@
 describe ServiceTemplate do
  include_examples "OwnershipMixin"

+  # let(:service_user) { instance_double("user") }


Probably don't need this line, haha 😄

Though, I would prefer to use the instance double if we can, but I'm guessing since it's commented out it didn't work for some reason.

eclarizio · 2018-08-23T21:52:42Z

spec/models/service/dialog_properties_spec.rb

+      end
+    end
+
+    # warn_on warn_in_days warn_in_hours warn_offset_days warn_offset_hours


Looks like this was a reminder to make these specs but they appear to be made above, so I think we can safely remove this.

d-m-u

Looks great except for the screenshotted weirdness.

tinaafitz

@gmcculloug Looks good. :-)

Changed to call create! Refactor create_service method to use create! with a block

gmcculloug · 2018-09-05T11:35:44Z

@d-m-u Updated with new commit to protect against times in the past.

@mkanoor Please review.

mkanoor · 2018-09-05T19:00:41Z

app/models/service/dialog_properties.rb

+    require_nested :Retirement
+
+    def initialize(options, user)
+      @attributes = {}


@gmcculloug Is the @attributes needed since we are parsing and just returning a hash back after the parse?

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

mkanoor · 2018-09-05T21:40:01Z

app/models/service/dialog_properties.rb

+
+    def parse
+      Service::DialogProperties::Retirement.parse(@options, @user).tap do |attributes|
+        @attributes[:name] = @options['dialog_service_name'] if @options['dialog_service_name'].present?


@gmcculloug
The local attributes is not being used, should @attributes be attributes here.

Definitely, it is fixed now.

Pass user so time parsing occurs in proper timezone.

miq-bot · 2018-09-05T22:12:34Z

Checked commits gmcculloug/manageiq@9ed98e9~...fd8709d with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
7 files checked, 1 offense detected

app/models/service/dialog_properties/retirement.rb

❗ - Line 34, Col 133 - Style/SpecialGlobalVars - Prefer $ERROR_INFO from the stdlib 'English' module (don't forget to require it) over $!.

Service retirement values from dialog (cherry picked from commit def0da5) Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1626475

simaishi · 2018-09-07T12:39:14Z

Fine backport details:

$ git log -1
commit ebe8cebff879016c920e4a6acab1cf8c19509a3d
Author: Madhu Kanoor <mkanoor@redhat.com>
Date:   Thu Sep 6 15:54:59 2018 -0400

    Merge pull request #16799 from gmcculloug/service_dialog_retirement
    
    Service retirement values from dialog
    (cherry picked from commit def0da55c254bd7386b1e1bd9c98f618e7ea7277)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1626475

Service retirement values from dialog (cherry picked from commit def0da5) Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1626474

simaishi · 2018-09-13T15:46:55Z

Gaprindashvili backport details:

$ git log -1
commit 253ce78dfd0e53839189829c4688d526902582a8
Author: Madhu Kanoor <mkanoor@redhat.com>
Date:   Thu Sep 6 15:54:59 2018 -0400

    Merge pull request #16799 from gmcculloug/service_dialog_retirement
    
    Service retirement values from dialog
    (cherry picked from commit def0da55c254bd7386b1e1bd9c98f618e7ea7277)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1626474

gmcculloug added enhancement lifecycle/services labels Jan 10, 2018

gmcculloug assigned mkanoor Jan 10, 2018

gmcculloug requested a review from tinaafitz January 10, 2018 22:55

miq-bot added the wip label Jan 10, 2018

gmcculloug mentioned this pull request Jan 10, 2018

Ability to retire a service from service dialog ManageIQ/manageiq-content#206

Closed

gmcculloug force-pushed the service_dialog_retirement branch from 7ea3cba to 620f44a Compare January 10, 2018 23:09

miq-bot added the unmergeable label Jan 31, 2018

gmcculloug force-pushed the service_dialog_retirement branch from 620f44a to abba0ff Compare February 1, 2018 13:48

miq-bot removed the unmergeable label Feb 1, 2018

gmcculloug force-pushed the service_dialog_retirement branch from abba0ff to 2291f38 Compare February 15, 2018 22:42

gmcculloug force-pushed the service_dialog_retirement branch from 2291f38 to e6f9ade Compare February 23, 2018 21:26

gmcculloug force-pushed the service_dialog_retirement branch from e6f9ade to 962aa1f Compare March 29, 2018 17:29

gmcculloug force-pushed the service_dialog_retirement branch from 962aa1f to c7ab760 Compare June 14, 2018 23:46

gmcculloug force-pushed the service_dialog_retirement branch from c7ab760 to 915ebc4 Compare July 10, 2018 13:12

gmcculloug force-pushed the service_dialog_retirement branch 2 times, most recently from e9b5aa7 to 4be0f14 Compare July 26, 2018 12:04

gmcculloug force-pushed the service_dialog_retirement branch 2 times, most recently from b81d047 to bf9aead Compare July 30, 2018 17:37

gmcculloug requested a review from eclarizio July 30, 2018 17:38

miq-bot added the blocker label Aug 7, 2018

gmcculloug force-pushed the service_dialog_retirement branch from bf9aead to 38597f2 Compare August 17, 2018 11:36

eclarizio reviewed Aug 23, 2018

View reviewed changes

d-m-u reviewed Sep 4, 2018

View reviewed changes

tinaafitz approved these changes Sep 4, 2018

View reviewed changes

Set owner properties before calling Service create

9ed98e9

Changed to call create! Refactor create_service method to use create! with a block

gmcculloug force-pushed the service_dialog_retirement branch from 8a41883 to 3f0d08b Compare September 5, 2018 11:31

gmcculloug added fine/yes labels Sep 5, 2018

d-m-u approved these changes Sep 5, 2018

View reviewed changes

mkanoor reviewed Sep 5, 2018

View reviewed changes

Move apply_dialog_settings logic into DialogProperties class

42b66cb

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

gmcculloug force-pushed the service_dialog_retirement branch from 3f0d08b to 01761c8 Compare September 5, 2018 20:50

mkanoor reviewed Sep 5, 2018

View reviewed changes

gmcculloug added 2 commits September 5, 2018 18:07

Support setting retires_on and retirement_warn for services from dialogs

741c4ca

Pass user so time parsing occurs in proper timezone.

Protect against setting retirement dates that have already past.

fd8709d

gmcculloug force-pushed the service_dialog_retirement branch from 01761c8 to fd8709d Compare September 5, 2018 22:09

mkanoor approved these changes Sep 6, 2018

View reviewed changes

mkanoor merged commit def0da5 into ManageIQ:master Sep 6, 2018

mkanoor added this to the Sprint 94 Ending Sept 10, 2018 milestone Sep 6, 2018

simaishi pushed a commit that referenced this pull request Sep 7, 2018

Merge pull request #16799 from gmcculloug/service_dialog_retirement

ebe8ceb

Service retirement values from dialog (cherry picked from commit def0da5) Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1626475

simaishi added fine/backported and removed fine/yes labels Sep 7, 2018

simaishi pushed a commit that referenced this pull request Sep 13, 2018

Merge pull request #16799 from gmcculloug/service_dialog_retirement

253ce78

Service retirement values from dialog (cherry picked from commit def0da5) Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1626474

simaishi added gaprindashvili/backported and removed gaprindashvili/yes labels Sep 13, 2018

This was referenced Apr 3, 2019

retirement_warn datatype set from dialog in service retirement isn't what we're testing #18618

Closed

Retirement warn in days only #18637

Merged

gmcculloug deleted the service_dialog_retirement branch January 1, 2020 23:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Service retirement values from dialog #16799

Service retirement values from dialog #16799

gmcculloug commented Jan 10, 2018 •

edited

Loading

gmcculloug commented Jan 10, 2018

miq-bot commented Jan 31, 2018

gmcculloug commented Jul 30, 2018

JPrause commented Aug 7, 2018

JPrause commented Aug 7, 2018

JPrause commented Aug 20, 2018

gmcculloug commented Aug 20, 2018

eclarizio left a comment •

edited

Loading

eclarizio Aug 23, 2018

eclarizio Aug 23, 2018

eclarizio Aug 23, 2018

eclarizio Aug 23, 2018

eclarizio Aug 23, 2018

d-m-u left a comment

tinaafitz left a comment

gmcculloug commented Sep 5, 2018

mkanoor Sep 5, 2018

mkanoor Sep 5, 2018

gmcculloug Sep 5, 2018

miq-bot commented Sep 5, 2018

simaishi commented Sep 7, 2018

simaishi commented Sep 13, 2018

Service retirement values from dialog #16799

Service retirement values from dialog #16799

Conversation

gmcculloug commented Jan 10, 2018 • edited Loading

Links

Steps for Testing/QA

gmcculloug commented Jan 10, 2018

miq-bot commented Jan 31, 2018

gmcculloug commented Jul 30, 2018

JPrause commented Aug 7, 2018

JPrause commented Aug 7, 2018

JPrause commented Aug 20, 2018

gmcculloug commented Aug 20, 2018

eclarizio left a comment • edited Loading

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

d-m-u left a comment

Choose a reason for hiding this comment

tinaafitz left a comment

Choose a reason for hiding this comment

gmcculloug commented Sep 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented Sep 5, 2018

simaishi commented Sep 7, 2018

simaishi commented Sep 13, 2018

gmcculloug commented Jan 10, 2018 •

edited

Loading

eclarizio left a comment •

edited

Loading