Move MiqGroup filters to Entitlements #8102

chrisarcand · 2016-04-19T18:22:20Z

Moves miq_group.filters to entitlements as-is.

chrisarcand · 2016-04-19T18:22:46Z

@miq-bot add_labels core, tenancy

chrisarcand · 2016-04-19T18:23:11Z

@chessbyte Jinx 😜

chrisarcand · 2016-04-19T18:53:00Z

r? @jrafanie

chessbyte · 2016-04-19T21:02:47Z

...viders-amazon/spec/models/manageiq/providers/amazon/cloud_manager/provision_workflow_spec.rb

-      admin.current_group.set_managed_filters([['/managed/cc/001']])
-      admin.current_group.set_belongsto_filters([])
-      admin.current_group.save
+      admin.current_group.entitlement = Entitlement.create!(:filters => {'managed'   => [['/managed/cc/001']],


should the hash keys be strings or symbols?

Strings; That's how they are [currently] serialized (text blob)

Sidenote; Doing this (shoving this hash literal everywhere) all over is definitely not ideal, but all of this will have to change at least twice with filters likely changing format (for merging) and with the introduction of multiple entitlements - so putting forth too much effort thinking of a more efficient way is moot.

gtanzillo · 2016-04-25T21:33:59Z

This looks good!! 👍 Would like for @jrafanie to 👍 before merging

jrafanie · 2016-04-26T14:39:03Z

app/models/entitlement.rb

+      end
+      self.filters["managed"].reject!(&:empty?)
+    end
+  end


Can we use a delete_if or a keep_if block to avoid iterating through the managed filters again? I believe you can check for empty? after deleting the filter

Was avoiding changing code that I'm just moving but solid points, 👍

Good point, it's up to you... you can do it after.

I guess I'll leave these methods be for now simply because they are
definitely one of the next things to change in tenancy work anyway.

On Tue, Apr 26, 2016 at 10:54 AM, Joe Rafaniello notifications@github.com
wrote:

In app/models/entitlement.rb
#8102 (comment):

def set_belongsto_filters(filter)

set_filters("belongsto", filter)

end

def remove_tag_from_managed_filter(filter_to_remove)

if get_managed_filters.present?

*category, _tag = filter_to_remove.split("/")

category = category.join("/")

self.filters["managed"].each do |filter|

next unless filter.first.starts_with?(category)

next unless filter.include?(filter_to_remove)

filter.delete(filter_to_remove)

end

self.filters["managed"].reject!(&:empty?)

end

end

Good point, it's up to you... you can do it after.

—
You are receiving this because you modified the open/close state.
Reply to this email directly or view it on GitHub
https://github.com/ManageIQ/manageiq/pull/8102/files/06caf4335f9a5014da4cbda051d2c9d481e31a9b#r61100438

Chris Arcand
@chrisarcand

Because nice things are nice and overidding is a nice thing.

Note that some of this is kinda hacky, we shouldn't be specifying their format all over... but this will _all_ change very soon and I'd like to get the migration done before Darga is cut.

Previously, MiqGroup#get_filters would return the blank filter format ({'managed' => [], 'belongsto' => []}) if no filters were set; this is changed in that its delegated to the group's entitlement. However, if the group doesn't yet have an entitlement, it now returns nil. This is because I don't want to specify empty format behavior in multiple places. Therefore, the one line change in RBAC besides the specs is due to expecting RBAC to return this full empty format (and the group now no longer returning that for it).

This is pretty hacky, but sadly the REST API is the only spot in the app where we unwisely use the MiqGroup#filters serialized column directly. That column has been moved to Entitlements. To allow the group API to continue setting filters by group for now, transform the filters attrs into attrs consumable by `accepts_nested_attributes_for` on MiqGroup. This will have to change soon; but to allow Entitlement filters to work for now, this keeps backwards compatibility.

miq-bot · 2016-04-26T20:36:58Z

Checked commits chrisarcand/manageiq@0947c51~...9f0749b with ruby 2.2.3, rubocop 0.37.2, and haml-lint 0.16.1
22 files checked, 5 offenses detected

app/models/entitlement.rb

🔶 - Line 21, Col 7 - Style/PredicateName - Rename has_filters? to filters?.
🔶 - Line 33, Col 7 - Style/AccessorMethodName - Do not prefix reader method names with get_.
🔶 - Line 37, Col 7 - Style/AccessorMethodName - Do not prefix reader method names with get_.
🔶 - Line 46, Col 7 - Style/AccessorMethodName - Do not prefix writer method names with set_.
🔶 - Line 50, Col 7 - Style/AccessorMethodName - Do not prefix writer method names with set_.

chrisarcand · 2016-04-26T22:23:18Z

@jrafanie All things addressed, this should be ready for another glance.

jrafanie · 2016-04-26T23:01:47Z

Looks good to me! 😎 👍

Yay darga branch, we can merge now.

kbrock · 2016-04-27T13:41:07Z

🎉 piece o' 🍰 ?

Caused by ManageIQ#8102 Without this, migrate fails when you go from "latest schema" down to: 20160317194215_remove_miq_user_role_from_miq_groups.rb: `PG::UndefinedColumn: ERROR: column entitlements.filters does not exist` Clearly, this isn't the right fix because then we'd have to reset column information everytime we add/remove a column in case "some other migration" tries to use that table's stub model. That would be bad. :cry:

chessbyte added refactoring core/tenancy core labels Apr 19, 2016

chrisarcand force-pushed the move-filters-to-entitlements branch from 33b96aa to 04b1e2e Compare April 19, 2016 18:47

Fryguy added the sql migration label Apr 19, 2016

chrisarcand closed this Apr 19, 2016

chrisarcand deleted the move-filters-to-entitlements branch April 19, 2016 19:28

chrisarcand restored the move-filters-to-entitlements branch April 19, 2016 19:28

chrisarcand reopened this Apr 19, 2016

chessbyte reviewed Apr 19, 2016
View reviewed changes

chessbyte added capablanca/no darga/no and removed capablanca/no labels Apr 20, 2016

chrisarcand force-pushed the move-filters-to-entitlements branch 2 times, most recently from 7331355 to e582652 Compare April 21, 2016 18:23

chrisarcand mentioned this pull request Apr 21, 2016

Reset column information after includes in migration #8167

Merged

Add entitlements filters column

0947c51

chrisarcand force-pushed the move-filters-to-entitlements branch from e582652 to 06caf43 Compare April 22, 2016 16:02

jrafanie reviewed Apr 26, 2016
View reviewed changes

chrisarcand force-pushed the move-filters-to-entitlements branch from e427b5e to bd5f4e0 Compare April 26, 2016 19:11

chrisarcand added 13 commits April 26, 2016 16:28

Move MiqGroup filters as-is to its entitlement

d6097c6

Remove filters column from MiqGroups

a13d593

Move filter logic over to Entitlements

408d8ce

Use subject in miq_group_spec

14e9536

Because nice things are nice and overidding is a nice thing.

Fix and cleanup MiqGroup spec per filters

50def10

Update MiqReport spec to use entitlement filters

e8ca95b

Fix Amazon provider gem specs given entitlement filters

91c43b2

Autosave entitlement from group

4146132

Fix vmdb specs utilizing entitlement filters

3c1aa98

Note that some of this is kinda hacky, we shouldn't be specifying their format all over... but this will _all_ change very soon and I'd like to get the migration done before Darga is cut.

Use setter methods for filters in spec

62d4aa0

Freeze this mutable constant (Rubocop)

9f0749b

chrisarcand force-pushed the move-filters-to-entitlements branch from bd5f4e0 to 9f0749b Compare April 26, 2016 20:32

jrafanie merged commit f62efba into ManageIQ:master Apr 26, 2016

jrafanie added this to the Sprint 40 Ending May 9, 2016 milestone Apr 26, 2016

jrafanie deleted the move-filters-to-entitlements branch April 26, 2016 23:02

jrafanie mentioned this pull request Apr 27, 2016

Hack: Reset rails' column cache for Entitlement for now. 😕 #8291

Merged

abonas mentioned this pull request Apr 28, 2016

Miq server doesn't start due to "undefined method 'filter' for Entitlement" #8315

Closed

jrafanie mentioned this pull request Nov 1, 2016

Expose miq_groups to Automate #12294

Merged

cben mentioned this pull request Oct 7, 2018

incomplete documentation for /api/groups — filters ManageIQ/manageiq-documentation#977

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move MiqGroup filters to Entitlements #8102

Move MiqGroup filters to Entitlements #8102

chrisarcand commented Apr 19, 2016 •

edited by jrafanie

Loading

chrisarcand commented Apr 19, 2016

chrisarcand commented Apr 19, 2016

chrisarcand commented Apr 19, 2016

chessbyte Apr 19, 2016

chrisarcand Apr 19, 2016

chrisarcand Apr 19, 2016

gtanzillo commented Apr 25, 2016 •

edited

Loading

jrafanie Apr 26, 2016

chrisarcand Apr 26, 2016

jrafanie Apr 26, 2016

chrisarcand Apr 26, 2016

miq-bot commented Apr 26, 2016

chrisarcand commented Apr 26, 2016

jrafanie commented Apr 26, 2016

kbrock commented Apr 27, 2016

Move MiqGroup filters to Entitlements #8102

Move MiqGroup filters to Entitlements #8102

Conversation

chrisarcand commented Apr 19, 2016 • edited by jrafanie Loading

chrisarcand commented Apr 19, 2016

chrisarcand commented Apr 19, 2016

chrisarcand commented Apr 19, 2016

chessbyte Apr 19, 2016

Choose a reason for hiding this comment

chrisarcand Apr 19, 2016

Choose a reason for hiding this comment

chrisarcand Apr 19, 2016

Choose a reason for hiding this comment

gtanzillo commented Apr 25, 2016 • edited Loading

jrafanie Apr 26, 2016

Choose a reason for hiding this comment

chrisarcand Apr 26, 2016

Choose a reason for hiding this comment

jrafanie Apr 26, 2016

Choose a reason for hiding this comment

chrisarcand Apr 26, 2016

Choose a reason for hiding this comment

miq-bot commented Apr 26, 2016

chrisarcand commented Apr 26, 2016

jrafanie commented Apr 26, 2016

kbrock commented Apr 27, 2016

chrisarcand commented Apr 19, 2016 •

edited by jrafanie

Loading

gtanzillo commented Apr 25, 2016 •

edited

Loading