Delete tag assignments when deleting a tag that is referenced in an assignment #16039

gtanzillo · 2017-09-25T21:58:13Z

Eg.

Tag to be deleted - /managed/environment/any1
Referenced in assignment by the tag /chargeback_rate/assigned_to/vm/tag/managed/environment/any1.
Which means that a chargeback rate (tagged with the latter) is assigned to any Vm
tagged with environment/any1. Should also be deleted

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

jrafanie · 2017-09-28T21:29:04Z

spec/models/classification_spec.rb

+
+        ent.destroy
+
+        expect(Tag.where(:name => assignment_tag).take).to be_nil


Do we need a test case showing that a similar tag in a different category isn't removed?

For example, "/chargeback_rate/assigned_to/vm/tag/managed/test_category2/test_entry"

The test I added to the AssignmentMixin tests that. Let me know if you think it still needs more. See https://github.com/ManageIQ/manageiq/pull/16039/files#diff-c96ad7d76c6072d9695e81a29f51e8b0R31

Ah, I guess I was more concerned with destroy not deleting things that are "close" but not exactly the same tag.

Ok, I'll add that test. Thanks!

jrafanie · 2017-09-28T21:30:52Z

app/models/mixins/assignment_mixin.rb

+  # with a given tag.
+  def self.all_assignments(tag = nil)
+    scope = Tag.where(["name LIKE ?", "%/#{AssignmentMixin::NAMESPACE_SUFFIX}/%"])
+    scope = scope.where(["name LIKE ?", "%#{tag}"]) if tag.present?


I feel like if there's a bug, it would be in the string matching. As long as we have good tests, it looks ok.

chrisarcand · 2017-09-29T02:19:28Z

app/models/mixins/assignment_mixin.rb

+
+  # This module method was not intended to be mixed into a class. It is used outside
+  # of any class that may mix this in, (Classification) to find the assignments associated
+  # with a given tag.


Wait wat? I don't understand; Why is this not just then a scope on Tag, as that's exactly what it does?

Actually, just to match the current convention that I'm seeing, why isn't it at last just included in the ClassMethods belong to be included on the class? Then it'd match the current .assignments method.

Chris, per your second point, I wanted to do it that way. But, the code in Classification that needs to delete these tags has no way of knowing which classes have included the mixin and would not know which classes on which to call the method.

I didn't consider making it a scope on Tag. I think that would work fine. But, then knowledge of how assignments are using tags would be added to the Tag class and I think it should stay in the mixin.

Ahh I see, I thought that Classification included this mixin.

In that case, you can make your intentions clear and without your comment by doing the following:

def all_assignments(tag = nil) # ... end module_function :all_assignments

This will signal that this method is intended to be called with the mixin itself as the receiver. It will be mixed in to instances of the class, but will be automatically marked as private. (https://ruby-doc.org/core-2.4.2/Module.html#method-i-module_function)

gtanzillo · 2017-09-29T14:40:51Z

@jrafanie I added that additional test. Please take a look when you can. Thanks.

jrafanie · 2017-09-29T14:57:37Z

spec/models/classification_spec.rb

+        Tag.create!(:name => assignment_tag)
+
+        cat.destroy
+


We probably want this just to show that it deletes the correct tag but not the others (on the next two lines)
expect(Tag.exists?(:name => assignment_tag)).to be false

…ssignment Eg. - Tag to be deleted - /managed/environment/any1 - Referenced in assignment by the tag /chargeback_rate/assigned_to/vm/tag/managed/environment/any1. Which means that a chargeback rate (tagged with the latter) is assigned to any Vm tagged with environment/any1. Should also be deleted https://bugzilla.redhat.com/show_bug.cgi?id=1483365

miq-bot · 2017-09-29T15:17:55Z

Checked commit gtanzillo@dba7bf9 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
4 files checked, 4 offenses detected

spec/models/classification_spec.rb

❗ - Line 103, Col 15 - Rails/DynamicFindBy - Use find_by instead of dynamic find_by_name.
❗ - Line 66, Col 15 - Rails/DynamicFindBy - Use find_by instead of dynamic find_by_name.
❗ - Line 76, Col 15 - Rails/DynamicFindBy - Use find_by instead of dynamic find_by_name.
❗ - Line 88, Col 15 - Rails/DynamicFindBy - Use find_by instead of dynamic find_by_name.

gtanzillo · 2017-09-29T15:48:30Z

@jrafanie @chrisarcand I've made all the suggested changes and I think this one is ready to go.

jrafanie

LGTM

Delete tag assignments when deleting a tag that is referenced in an assignment (cherry picked from commit fc67deb) https://bugzilla.redhat.com/show_bug.cgi?id=1497746

simaishi · 2017-10-02T14:58:51Z

Fine backport details:

$ git log -1
commit 6b8f0a802346f37c89143114135d0fce1440189f
Author: Joe Rafaniello <jrafanie@users.noreply.github.com>
Date:   Fri Sep 29 14:00:32 2017 -0400

    Merge pull request #16039 from gtanzillo/delete-tag-assignments
    
    Delete tag assignments when deleting a tag that is referenced in an assignment
    (cherry picked from commit fc67deb5c3a1eca38574e9001ec52f933da0b112)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1497746

Delete tag assignments when deleting a tag that is referenced in an assignment (cherry picked from commit fc67deb) https://bugzilla.redhat.com/show_bug.cgi?id=1497748

simaishi · 2017-10-02T15:08:28Z

Euwe backport details:

$ git log -1
commit 3e1b9f9ee92704b7a620d8d11e937374575dfe09
Author: Joe Rafaniello <jrafanie@users.noreply.github.com>
Date:   Fri Sep 29 14:00:32 2017 -0400

    Merge pull request #16039 from gtanzillo/delete-tag-assignments
    
    Delete tag assignments when deleting a tag that is referenced in an assignment
    (cherry picked from commit fc67deb5c3a1eca38574e9001ec52f933da0b112)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1497748

Delete tag assignments when deleting a tag that is referenced in an assignment (cherry picked from commit fc67deb) https://bugzilla.redhat.com/show_bug.cgi?id=1497746

gtanzillo added bug core/chargeback wip blocker euwe/yes labels Sep 25, 2017

gtanzillo force-pushed the delete-tag-assignments branch 3 times, most recently from b2c2a3b to a42c7f9 Compare September 28, 2017 14:25

gtanzillo removed the wip label Sep 28, 2017

gtanzillo changed the title ~~[WIP] Delete tag assignments when deleting a tag that is referenced in an assignment~~ Delete tag assignments when deleting a tag that is referenced in an assignment Sep 28, 2017

gtanzillo force-pushed the delete-tag-assignments branch 2 times, most recently from 651deac to 706ddb8 Compare September 28, 2017 14:56

gtanzillo assigned jrafanie Sep 28, 2017

jrafanie reviewed Sep 28, 2017

View reviewed changes

chrisarcand reviewed Sep 29, 2017

View reviewed changes

gtanzillo force-pushed the delete-tag-assignments branch from 706ddb8 to 2e51024 Compare September 29, 2017 13:43

jrafanie reviewed Sep 29, 2017

View reviewed changes

gtanzillo force-pushed the delete-tag-assignments branch from 2e51024 to dba7bf9 Compare September 29, 2017 15:15

jrafanie approved these changes Sep 29, 2017

View reviewed changes

jrafanie merged commit fc67deb into ManageIQ:master Sep 29, 2017

jrafanie added this to the Sprint 70 Ending Oct 2, 2017 milestone Sep 29, 2017

simaishi added the fine/backported label Oct 2, 2017

simaishi removed the fine/yes label Oct 2, 2017

simaishi added euwe/backported and removed euwe/yes labels Oct 2, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Delete tag assignments when deleting a tag that is referenced in an assignment #16039

Delete tag assignments when deleting a tag that is referenced in an assignment #16039

gtanzillo commented Sep 25, 2017 •

edited

Loading

jrafanie Sep 28, 2017

gtanzillo Sep 28, 2017

jrafanie Sep 28, 2017

gtanzillo Sep 28, 2017

jrafanie Sep 28, 2017

chrisarcand Sep 29, 2017

gtanzillo Sep 29, 2017

chrisarcand Sep 29, 2017

gtanzillo commented Sep 29, 2017

jrafanie Sep 29, 2017

miq-bot commented Sep 29, 2017

gtanzillo commented Sep 29, 2017

jrafanie left a comment

simaishi commented Oct 2, 2017

simaishi commented Oct 2, 2017


		ent.destroy

		expect(Tag.where(:name => assignment_tag).take).to be_nil

Delete tag assignments when deleting a tag that is referenced in an assignment #16039

Delete tag assignments when deleting a tag that is referenced in an assignment #16039

Conversation

gtanzillo commented Sep 25, 2017 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gtanzillo commented Sep 29, 2017

Choose a reason for hiding this comment

miq-bot commented Sep 29, 2017

gtanzillo commented Sep 29, 2017

jrafanie left a comment

Choose a reason for hiding this comment

simaishi commented Oct 2, 2017

simaishi commented Oct 2, 2017

gtanzillo commented Sep 25, 2017 •

edited

Loading