Check for both owner email and requester email for user quota. #230

billfitzgerald0120 · 2017-12-15T20:27:56Z

Changed used method to check for owner email or requester email when quota is set to user.
Previously the method was only looking for owner_email and when that wasn't available we encountered an error.

The method now will raise an error if both are not found. If you are running a service provision which doesn't allow a requester email
and the user doesn't have an email address, we will raise this error:

ERROR - Owner email not specified for User Quota

Added tests for all possible email values for user quota.

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

billfitzgerald0120 · 2017-12-15T20:31:50Z

@miq-bot add_label bug, fine/yes, gaprindashvili/yes, blocker

billfitzgerald0120 · 2017-12-15T20:32:02Z

@miq-bot assign @gmcculloug

billfitzgerald0120 · 2017-12-15T20:38:50Z

@tinaafitz Please Review

gmcculloug

Overall this looks good, a few cleanup comments.

gmcculloug · 2017-12-19T18:39:24Z

content/automate/ManageIQ/System/CommonMethods/QuotaMethods.class/__methods__/used.rb

@@ -70,6 +85,6 @@ def merge_counts(quota_used, quota_active)
  end
 end

-if __FILE__ == $PROGRAM_NAME
+if $PROGRAM_NAME == __FILE__


@billfitzgerald0120 Is there any reason for this change? All other methods that use this line have it the original way and we should remain consistent.

Rubocop didn't like this

ah, yes. Yoda condition it is.

gmcculloug · 2017-12-19T18:43:42Z

content/automate/ManageIQ/System/CommonMethods/QuotaMethods.class/__methods__/used.rb

@@ -1,5 +1,5 @@
 # frozen_string_literal: true
-#
+


Rubocop didn't like this

For consistency with other methods I would just add a new-line instead of dropping the comment line:

# frozen_string_literal: true # # Description: calculate entity used quota values #

gmcculloug · 2017-12-19T20:05:06Z

spec/content/automate/ManageIQ/System/CommonMethods/QuotaMethods.class/__methods__/used_spec.rb

+      @miq_provision_request.options[:owner_email] = owner_email
+      @user.email = requester_email
+      @miq_provision_request.save
+      @user.save


You can update and save each of these objects in a single line.

@miq_provision_request.update(:options => @miq_provision_request.options.merge(:owner_email => owner_email)) @user.update(:email => requester_email)

gmcculloug · 2017-12-19T20:06:22Z

spec/content/automate/ManageIQ/System/CommonMethods/QuotaMethods.class/__methods__/used_spec.rb

+      @user.save
+      expect(svc_miq_request).to receive(:check_quota).with(active_method).and_return(active_counts_hash)
+      described_class.new(ae_service).main
+      expect(ae_service.root['quota_used']).to include(result_counts_hash)


Please add a new-line between the test setup and the expect.

gmcculloug · 2017-12-19T20:08:00Z

spec/content/automate/ManageIQ/System/CommonMethods/QuotaMethods.class/__methods__/used_spec.rb

+    it "when no owner or requester email" do
+      @miq_provision_request.options[:owner_email] = nil
+      @miq_provision_request.save
+      expect { described_class.new(ae_service).main }.to raise_error(errormsg)


Same things for this test, Update/save in a single line and add a new-line between setup and expect.

it "when no owner or requester email" do @miq_provision_request.update(:options => @miq_provision_request.options.merge(:owner_email => nil)) expect { described_class.new(ae_service).main }.to raise_error(errormsg) end

billfitzgerald0120 · 2017-12-19T21:52:28Z

@gmcculloug Made all changes as requested. Please Review

Changed used method to check for owner email or requester email when quota is set to user. Previously the method was only looking for owner_email and when that wasn't available we encountered an error. The method now will raise an error if both are not found. If you are running a service provision which doesn't allow a requester email and the user doesn't have an email address, we will raise this error: ERROR - Owner email not specified for User Quota Added tests for all possible email values for user quota. https://bugzilla.redhat.com/show_bug.cgi?id=1509977

miq-bot · 2017-12-19T23:23:45Z

Checked commit billfitzgerald0120@784d666 with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🏆

tinaafitz

@billfitzgerald0120 Looks good.

Check for both owner email and requester email for user quota. (cherry picked from commit 25317bf) https://bugzilla.redhat.com/show_bug.cgi?id=1530644

simaishi · 2018-01-03T16:48:09Z

Gaprindashvili backport details:

$ git log -1
commit 9c904b621f5f20a7919bf271f90a3341057b496a
Author: Greg McCullough <gmccullo@redhat.com>
Date:   Wed Dec 20 09:19:11 2017 -0500

    Merge pull request #230 from billfitzgerald0120/user_quota_email
    
    Check for both owner email and requester email for user quota.
    (cherry picked from commit 25317bff448819df562122d670999db6b2813b94)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1530644

Check for both owner email and requester email for user quota. (cherry picked from commit 25317bf) https://bugzilla.redhat.com/show_bug.cgi?id=1531161

simaishi · 2018-01-04T18:46:34Z

Fine backport details:

$ git log -1
commit 5bb99090f2599cbd7138047d5ad9698d17a4b588
Author: Greg McCullough <gmccullo@redhat.com>
Date:   Wed Dec 20 09:19:11 2017 -0500

    Merge pull request #230 from billfitzgerald0120/user_quota_email
    
    Check for both owner email and requester email for user quota.
    (cherry picked from commit 25317bff448819df562122d670999db6b2813b94)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1531161

miq-bot added blocker bug fine/yes labels Dec 15, 2017

miq-bot assigned gmcculloug Dec 15, 2017

billfitzgerald0120 force-pushed the user_quota_email branch from 0b4d3f9 to b27ca56 Compare December 19, 2017 17:36

gmcculloug reviewed Dec 19, 2017

View reviewed changes

tinaafitz mentioned this pull request Dec 19, 2017

Fix email issue in miq_provision_quota_mixin active_provision by_owner method. ManageIQ/manageiq#16693

Merged

billfitzgerald0120 force-pushed the user_quota_email branch from b27ca56 to db64f00 Compare December 19, 2017 21:49

billfitzgerald0120 force-pushed the user_quota_email branch from db64f00 to 5ca8d0c Compare December 19, 2017 22:06

gmcculloug requested a review from tinaafitz December 19, 2017 22:23

billfitzgerald0120 force-pushed the user_quota_email branch from 5ca8d0c to 784d666 Compare December 19, 2017 23:14

tinaafitz approved these changes Dec 20, 2017

View reviewed changes

gmcculloug merged commit 25317bf into ManageIQ:master Dec 20, 2017

gmcculloug added this to the Sprint 76 Ending Jan 1, 2018 milestone Dec 20, 2017

simaishi added gaprindashvili/backported and removed gaprindashvili/yes labels Jan 3, 2018

simaishi added fine/backported and removed fine/yes labels Jan 4, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check for both owner email and requester email for user quota. #230

Check for both owner email and requester email for user quota. #230

billfitzgerald0120 commented Dec 15, 2017

billfitzgerald0120 commented Dec 15, 2017

billfitzgerald0120 commented Dec 15, 2017

billfitzgerald0120 commented Dec 15, 2017

gmcculloug left a comment

gmcculloug Dec 19, 2017

billfitzgerald0120 Dec 19, 2017

gmcculloug Dec 19, 2017

gmcculloug Dec 19, 2017

billfitzgerald0120 Dec 19, 2017

gmcculloug Dec 19, 2017

gmcculloug Dec 19, 2017

billfitzgerald0120 Dec 19, 2017

gmcculloug Dec 19, 2017

billfitzgerald0120 Dec 19, 2017

gmcculloug Dec 19, 2017

billfitzgerald0120 Dec 19, 2017

billfitzgerald0120 commented Dec 19, 2017

miq-bot commented Dec 19, 2017

tinaafitz left a comment

simaishi commented Jan 3, 2018

simaishi commented Jan 4, 2018

Check for both owner email and requester email for user quota. #230

Check for both owner email and requester email for user quota. #230

Conversation

billfitzgerald0120 commented Dec 15, 2017

billfitzgerald0120 commented Dec 15, 2017

billfitzgerald0120 commented Dec 15, 2017

billfitzgerald0120 commented Dec 15, 2017

gmcculloug 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

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

billfitzgerald0120 commented Dec 19, 2017

miq-bot commented Dec 19, 2017

tinaafitz left a comment

Choose a reason for hiding this comment

simaishi commented Jan 3, 2018

simaishi commented Jan 4, 2018