fixes #6150 - users need taxonomy added on "all Users" #4111

kgaikwad · 2016-12-14T13:12:52Z

With this commit, if "All users" is ticked then
locations and organizations are added to users. On edit user page,
you can not edit this taxonomy as they are disabled with tooltip
"Select all option enabled for this taxonomy".

Likewise, it will fix 'select all' option functionality for other ignore_types as well.

mention-bot · 2016-12-14T13:12:55Z

@kgaikwad, thanks for your PR! By analyzing the history of the files in this pull request, we identified @isratrade, @tbrisker and @amirfefer to be potential reviewers.

houndci-bot · 2016-12-14T13:13:04Z

db/migrate/20161212121640_update_taxonomies_with_users.rb

+    say "This Migration will take time for updating organization & location records with all users for which ignore_types includes 'User' resource."
+    Rake::Task['taxonomy:update_taxonomy'].invoke
+  end
+


Trailing whitespace detected.

houndci-bot · 2017-01-25T11:02:29Z

app/models/taxonomy.rb

@@ -51,6 +51,8 @@ def self.inherited(child)
    end
  }

+  scope :enabled_select_all_taxonomies, -> (taxable_type){where("ignore_types LIKE ?", "%#{[taxable_type].to_yaml}%") }


Do not use spaces between -> and opening brace in lambda literals

kgaikwad · 2017-01-25T13:34:10Z

I have updated the pull-request.
codeclimate is showing complexity warnings for taxonomix.rb file.
But I am not getting how to resolve it.
Please provide me some pointers so that I will do further modifications.

kgaikwad · 2017-02-10T10:05:23Z

I have updated the pull-request with modifications to solve complexity warning for file taxonomix.rb

ares · 2017-07-19T10:54:37Z

Thanks @kgaikwad, could you please rebase the PR?

kgaikwad · 2017-07-19T10:58:48Z

@ares ,
Yes, I will update PR soon.

houndci-bot · 2017-07-19T12:48:38Z

app/helpers/form_helper.rb

-        klass = Location
-      else
-        klass = nil
+    options.merge!(:size => "col-md-10")


Use options[:size] = "col-md-10" instead of options.merge!(:size => "col-md-10").

kgaikwad · 2017-07-19T12:54:53Z

@ares ,
Done. I have updated the PR.
Please let me know if any further changes required.

ares

Thanks Kavita, patches improving this area are highly appreciated. In general I think this improves the current situation but does not solve it completely.

I think that it could get out of sync again in case a new organization or location ignoring users is added after some user already exists. Perhaps some callback on Taxonomy model should be added that will make sure all resources are linked if they're ignored.

Last but not least, this fixes the issue for User model only. Could it be generalized in taxonomix.rb so it applies to all taxable resources? If it's implemented this way and we can rely on having proper relations in DB between resource and taxonomy, I think it could simplify the existing code

I think more tests would be helpful since this is area is prone to errors. Covering all new methods and a test verifying that after taxonomy starts to ignore a resource class, every such $resource.taxonomies includes this taxonomy

ares · 2017-07-20T11:59:32Z

app/models/user.rb

@@ -560,6 +561,19 @@ def verify_current_password
    end
  end

+  # assign all taxonomies for which ignore_type includes 'User' resource
+  def add_select_all_taxonomies
+    if self.new_record?


the condition could be moved to after_initialize :add_select_all_taxonomies, :if => :new_record? so it's more obvious it does not happen for existing users

ares · 2017-07-20T12:02:08Z

app/views/taxonomies/_loc_org_tabs.html.erb

            {'data-descendants' => obj.children_of_selected_organization_ids.to_json,
-             'data-useds' => obj.used_organization_ids.to_json }.merge!(html_options[:organization]) %>
+             'data-useds' => obj.used_organization_ids.to_json,
+	     'data-usedall' => enabled_select_all_organization_ids.to_json }.merge!(html_options[:organization]) %>


indentation is a bit off,
also, usedall should be split to data-used-all

ares · 2017-07-20T12:04:03Z

lib/tasks/taxonomy.rake

+  desc <<-END_DESC
+	This task will update organization & location records with all users for which ignore_types includes 'User' resource.
+  END_DESC
+  task :update_taxonomy => :environment do


the rake task should probably run in User.anonymous_admin do ... end (required since recent changes, probably was not required back then)

Tried the scenario with newly created DB, this migration fails because user ANONYMOUS_ADMIN not being present at that time. Added condition to handle this in rake task but is there any better way to run the rake task in case this user is not present?

The admin user should exist when this task is needed, since admins are seeded before taxonomies. If the database is not seeded at all, this task shouldn't be needed and can be skipped. The condition could check if Organization.any? or if Location.any? but it's similar to what you have already. So I guess it can remain as is.

Okay then I am keeping the condition as it is.
Thank you @ares

ares · 2017-07-20T12:08:02Z

app/models/user.rb

@@ -100,6 +100,7 @@ def self.name_format

  after_create :welcome_mail
  after_create :set_default_widgets
+  after_initialize :add_select_all_taxonomies


This seems to work for new users but I think we should do similar check for existing ones, e.g. when I update some user through API and select subset of organizations. I can't do this through WebUI because of disabled fields but API/hammer could still do it.

kgaikwad · 2017-08-17T09:47:28Z

@ares ,

Thank you for explaining in brief. As per your suggestions, I have modified the code and added before_save hook for all ignore_types as well as on taxonomy.
I have queries regarding few points:

As you mentioned that code modifications will simplify the existing code.
Instead of querying taxable_class with new changes we can query on taxonomy itself.
i.e. taxonomy.send("#{base_class.name.underscore}_ids")
Please correct me if I misunderstood something here.
In taxonomy.rb,
_ids methods are overwritten where ignore_types are returned by querying on type class instead of DB relations. For users, all with unscoped.except_admin.except_hidden and for other types, it simply returns all records.
It may cause conflicts in above mentioned case and other areas.
Please guide me on this.

Pending Work - I will add test cases once code changes done.

theforeman-bot · 2019-07-24T08:13:20Z

@kgaikwad, this pull request is currently not mergeable. Please rebase against the develop branch and push again.

If you have a remote called 'upstream' that points to this repository, you can do this by running:

    $ git pull --rebase upstream develop

This message was auto-generated by Foreman's prprocessor

ares · 2019-10-10T08:10:53Z

@kgaikwad hello, is this something we could revisit after 1.24 branching? Do you think you'll have capacity to finish it? Improvements in this area are more than welcome.

kgaikwad · 2019-10-10T14:13:08Z

@kgaikwad hello, is this something we could revisit after 1.24 branching? Do you think you'll have capacity to finish it? Improvements in this area are more than welcome.

Thank you @ares.
Yeah, after 1.24 branching we can revisit this as I am not sure about how many changes will be required to fix this "all " options and what will be the impact on current behaviour.

app/models/taxonomy.rb

app/helpers/form_helper.rb

app/models/concerns/taxonomix.rb

kgaikwad · 2020-01-02T14:03:45Z

@ares,
I have done with rebase.
Tests are green. As I mentioned above, I would like to know if any additional migration or rake task changes are required so that it will run smoothly on any environment.

EDIT - I have tested with few general scenarios:

Tested with organization/location where select all option is enabled for few ignore_types.
Tested behavior of child organization/location when respective parent is having this "select all" option enabled against ignore_types.
DB changes as per UI modification while creating/updating records having organizations/locations with such options.
If any other complex scenarios that I should test then please let me know.

bkearney · 2020-04-02T13:53:00Z

@ares are we still waiting on the updates from @kgaikwad

With this commit, if "All users" is ticked then locations and organizations are added to users. On edit user page, you can not edit this taxonomy as they are disabled with tooltip "Select all option enabled for this taxonomy".

houndci-bot · 2020-04-02T15:02:01Z

test/models/taxonomy_test.rb

@@ -73,4 +73,24 @@ class TaxonomyTest < ActiveSupport::TestCase
    location.save
    assert_match /expecting organizations/, location.errors.messages[:organizations].first
  end
+
+  test "#taxonomy_ids_by_ignore_type should return ids of taxonomy_type for which 'Select All' option is checked for a resource." do
+    FactoryBot.create(:organization, :ignore_types => [ 'User' ])


Layout/SpaceInsideArrayLiteralBrackets: Do not use space inside array brackets.

houndci-bot · 2020-04-02T15:02:01Z

app/models/taxonomy.rb

+      klass_ids_meth = klass.underscore + '_ids'
+      existing_ids = self.send(klass_ids_meth)
+      next if (klass_obj_ids - existing_ids).blank?
+      self.send("#{klass_ids_meth}=", klass_obj_ids)
