Do not delete report if task associated with this report deleted #15134

yrudman · 2017-05-17T20:06:33Z

Issue: Deleting task (in task management screen) also deleting report linked to that task

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

Fixes:

remove :dependent => :destroy on association between MiqTask and MiqReportResult
allow to access saved report after associated task deleted: MiqReportResult#status now returns Completed if associated task deleted

AFTER: on screenshot first report has task associated with it and for last 2 reports (one successful and one failed) associated task was deleted

@miq-bot add-label core, bug

yrudman · 2017-05-22T15:22:34Z

@gtanzillo rebased and ready for the second review

gtanzillo · 2017-05-22T21:56:45Z

app/models/miq_report_result.rb

@@ -32,11 +30,16 @@ def result_set
  end

  def status
-    MiqTask.human_status(state_or_status)
+    task = MiqTask.find_by(:id=>miq_task_id)


You should be able to reference the association miq_task here instead of doing MiqTask.find_by(:id=>miq_task_id)

@gtanzillo right, no need for find_by, changed

miq-bot · 2017-05-24T12:51:50Z

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

jrafanie · 2017-05-30T15:54:50Z

app/models/miq_report_result.rb

@@ -36,7 +36,7 @@ def status
  end

  def status_message
-    miq_task.nil? ? _("Report results are no longer available") : miq_task.message
+    miq_task.nil? ? _("Task associated with this report no longer available") : miq_task.message


I believe this is logged or in the UI so, maybe The task associated with this report is no longer available"

jrafanie · 2017-05-30T15:56:37Z

spec/models/miq_report_result_spec.rb

@@ -125,6 +125,33 @@
    end
  end

+  describe "#state" do


the tests here are testing #status, not #state

jrafanie · 2017-05-30T15:58:07Z

app/models/miq_report_result.rb

@@ -11,10 +11,8 @@ class MiqReportResult < ApplicationRecord
  serialize :report

  virtual_delegate :description, :to => :miq_group, :prefix => true, :allow_nil => true
-  virtual_delegate :state_or_status, :to => "miq_task", :allow_nil => true
-  virtual_attribute :status, :string, :uses => :state_or_status


I can't tell, is this required for this PR? How do you know it's unused? Can you locate the last caller or why it's unused?

Search for string state_or_statusacross all projects: found only in MiqTask and it is not invocation of MiqTaskResult.state_or_status

jrafanie · 2017-05-30T16:00:48Z

@yrudman your commit message first line (subject) are too long:

Try to keep them under 70 characters

jrafanie · 2017-05-30T16:02:50Z

Are you comfortable squashing commits? The second and fourth commit are related in that the fourth undoes the way the second commit works and does it differently. They should probably be squashed since the first commit doesn't really stand on it's own anymore.

yrudman · 2017-05-30T22:40:30Z

@jrafanie yes, separate commits looks confusing, rebased to 1 commit...

jrafanie · 2017-05-31T16:00:13Z

spec/models/miq_report_result_spec.rb

+      expect(miq_report_result.status).to eq "Running"
+    end
+
+    it "returns 'Complete' if report  generated and associated task exists" do


there's two spaces after report here

jrafanie

LGTM with minor nitpick. I don't understand the use case of deleting tasks so I'm not sure what's expected, but this definitely solves the BZ.

BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1450272 Changes: - removed :dependent => :destroy on miq_report_result association in MiqTask - refactor MiqReportResult#status to return 'Complete' if task asociated with report deleted - added test coverage for MiqReportResult#status - remove not used virtual_delegate to MiqTask#state_or_status

miq-bot · 2017-06-01T18:08:02Z

Checked commit yrudman@fa4ebc6 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
3 files checked, 0 offenses detected
Everything looks fine. 🍪

jrafanie · 2017-06-01T18:45:20Z

@gtanzillo please review #15134 (review)... I'm good with the change but don't understand the use case of deleting tasks to know what the expected behavior is.

gtanzillo

I'm good with this change. 👍 Thanks for reviewing @jrafanie

…deleted Do not delete report if task associated with this report deleted (cherry picked from commit c17d043) https://bugzilla.redhat.com/show_bug.cgi?id=1460394

simaishi · 2017-06-12T17:15:27Z

Fine backport details:

$ git log -1
commit 403b51d97b0c730db157c065c704e0994c9704fa
Author: Gregg Tanzillo <[email protected]>
Date:   Thu Jun 1 17:57:11 2017 -0400

    Merge pull request #15134 from yrudman/fix-deleting-report-when-task-deleted
    
    Do not delete report if task associated with this report deleted
    (cherry picked from commit c17d0430bd143dbf3e40c131c2dc831bd4958bae)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1460394

…llow-up to ManageIQ/manageiq#15134. BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1450272

miq-bot added wip bug core labels May 17, 2017

yrudman mentioned this pull request May 18, 2017

[WIP] Allow to access saved report after associated task deleted ManageIQ/manageiq-ui-classic#1377

Closed

yrudman changed the title ~~[WIP] Do not delete report if task associated with this report deleted~~ Do not delete report if task associated with this report deleted May 18, 2017

miq-bot removed the wip label May 18, 2017

miq-bot assigned gtanzillo May 18, 2017

yrudman changed the title ~~Do not delete report if task associated with this report deleted~~ [WIP] Do not delete report if task associated with this report deleted May 18, 2017

miq-bot added the wip label May 18, 2017

yrudman force-pushed the fix-deleting-report-when-task-deleted branch from 79ce384 to fd9a6b7 Compare May 22, 2017 12:47

yrudman changed the title ~~[WIP] Do not delete report if task associated with this report deleted~~ Do not delete report if task associated with this report deleted May 22, 2017

miq-bot removed the wip label May 22, 2017

yrudman force-pushed the fix-deleting-report-when-task-deleted branch from fd9a6b7 to 0945005 Compare May 22, 2017 13:09

yrudman force-pushed the fix-deleting-report-when-task-deleted branch from 0945005 to d692cb1 Compare May 22, 2017 16:10

gtanzillo reviewed May 22, 2017

View reviewed changes

yrudman force-pushed the fix-deleting-report-when-task-deleted branch from d692cb1 to ef6922c Compare May 23, 2017 12:36

miq-bot added the unmergeable label May 24, 2017

yrudman force-pushed the fix-deleting-report-when-task-deleted branch from ef6922c to 063e202 Compare May 24, 2017 15:39

miq-bot added fine/yes and removed unmergeable labels May 24, 2017

jrafanie reviewed May 30, 2017

View reviewed changes

yrudman force-pushed the fix-deleting-report-when-task-deleted branch from 063e202 to 246f6a6 Compare May 30, 2017 22:36

jrafanie reviewed May 31, 2017

View reviewed changes

jrafanie approved these changes May 31, 2017

View reviewed changes

yrudman force-pushed the fix-deleting-report-when-task-deleted branch from 246f6a6 to fa4ebc6 Compare June 1, 2017 18:05

gtanzillo approved these changes Jun 1, 2017

View reviewed changes

gtanzillo added this to the Sprint 62 Ending Jun 5, 2017 milestone Jun 1, 2017

gtanzillo merged commit c17d043 into ManageIQ:master Jun 1, 2017

yrudman deleted the fix-deleting-report-when-task-deleted branch June 2, 2017 11:26

lpichler mentioned this pull request Jun 2, 2017

Use for fetching report result status of MiqTask ManageIQ/manageiq-ui-classic#1482

Merged

yrudman mentioned this pull request Jun 5, 2017

Allow to see saved reports after linked MiqTask deleted ManageIQ/manageiq-ui-classic#1488

Merged

simaishi added fine/backported and removed fine/yes labels Jun 12, 2017

simaishi mentioned this pull request Jun 12, 2017

Fix spec to use correct status of miq_task (CI failure) ManageIQ/manageiq-ui-classic#1481

Merged

mzazrivec pushed a commit to ManageIQ/manageiq-ui-classic that referenced this pull request Aug 24, 2017

allow to see saved chargeback report after linked MiqTask deleted, fo…

48e3659

…llow-up to ManageIQ/manageiq#15134. BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1450272

nimrodshn pushed a commit to ilackarms/manageiq-ui-classic that referenced this pull request Aug 27, 2017

allow to see saved chargeback report after linked MiqTask deleted, fo…

6b66e31

…llow-up to ManageIQ/manageiq#15134. BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1450272

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not delete report if task associated with this report deleted #15134

Do not delete report if task associated with this report deleted #15134

yrudman commented May 17, 2017 •

edited

Loading

yrudman commented May 22, 2017

gtanzillo May 22, 2017

yrudman May 23, 2017

miq-bot commented May 24, 2017

jrafanie May 30, 2017

yrudman May 30, 2017

jrafanie May 30, 2017

yrudman May 30, 2017

jrafanie May 30, 2017

yrudman May 30, 2017

jrafanie commented May 30, 2017

jrafanie commented May 30, 2017

yrudman commented May 30, 2017

jrafanie May 31, 2017

yrudman Jun 1, 2017

jrafanie left a comment

miq-bot commented Jun 1, 2017

jrafanie commented Jun 1, 2017

gtanzillo left a comment

simaishi commented Jun 12, 2017

Do not delete report if task associated with this report deleted #15134

Do not delete report if task associated with this report deleted #15134

Conversation

yrudman commented May 17, 2017 • edited Loading

yrudman commented May 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented May 24, 2017

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

jrafanie commented May 30, 2017

jrafanie commented May 30, 2017

yrudman commented May 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrafanie left a comment

Choose a reason for hiding this comment

miq-bot commented Jun 1, 2017

jrafanie commented Jun 1, 2017

gtanzillo left a comment

Choose a reason for hiding this comment

simaishi commented Jun 12, 2017

yrudman commented May 17, 2017 •

edited

Loading