DMPRoadmap / roadmap

DCC/UC3 collaboration for a data management planning tool
MIT License
102 stars 109 forks source link

An issue with GuidanceController admin_index() method #3055

Open johnpinto1 opened 2 years ago

johnpinto1 commented 2 years ago

In app/models/guidance_group.rb we have the method

def self.create_org_default(org) GuidanceGroup.create!( name: org.abbreviation? ? org.abbreviation : org.name, org: org, optional_subset: false ) end

In app/controllers/guidances_controller.rb the above method is called by method

def ensure_default_group(org) return unless org.managed? return if org.guidance_groups.where(optional_subset: false).present?

GuidanceGroup.create_org_default(org)

end

which is called by the controller method

GET /org/admin/guidance/:id/admin_index def admin_index authorize Guidance @guidances = Guidance.by_org(current_user.org) .includes(:guidance_group, :themes).page(1) ensure_default_group(current_user.org) @guidance_groups = GuidanceGroup.by_org(current_user.org).page(1) end

In DCC we had the following issue comment https://github.com/DigitalCurationCentre/DMPonline-Service/issues/625#issuecomment-953013169

If the GuidanceGroup has variable optional_subset: true

gg = GuidanceGroup.where(name: "MIUN") GuidanceGroup Load (0.9ms) SELECT "guidance_groups".* FROM "guidance_groups" WHERE "guidance_groups"."name" = $1 LIMIT $2 [["name", "MIUN"], ["LIMIT", 11]] => #<ActiveRecord::Relation [#<GuidanceGroup id: 480245895, name: "MIUN", org_id: 1071580137, created_at: "2021-04-26 12:29:36", updated_at: "2021-04-26 12:29:59", optional_subset: true, published: false>]>

This causes the method ensure_default_group in *app/controllers/guidances_controller.rb fails because of error ActiveRecord::RecordInvalid (Validation failed: Name must be unique)** we cannot access the Guidance pages by an Admin.

irb(main):002:0> GuidanceGroup.create!(name:mu.abbreviation, org: mu, optional_subset: false) (CALL THAT FAILS IN CODE) (0.1ms) SAVEPOINT active_record_1 GuidanceGroup Exists (0.4ms) SELECT 1 AS one FROM "guidance_groups" WHERE "guidance_groups"."name" = $1 AND "guidance_groups"."org_id" = $2 LIMIT $3 [["name", "MIUN"], ["org_id", 1071580137], ["LIMIT", 1]] (0.2ms) ROLLBACK TO SAVEPOINT active_record_1 Traceback (most recent call last): 1: from (irb):2 ActiveRecord::RecordInvalid (Validation failed: Name must be unique)

This suggest an issue for the method ensure_default_group in *app/controllers/guidances_controller.rb**.

@briri & @raycarrick-ed I am not clear how the GuidanceGroup GuidanceGroup.where(name: "MIUN") got into state optional_subset: true => #<ActiveRecord::Relation [#<GuidanceGroup id: 480245895, name: "MIUN", org_id: 1071580137, created_at: "2021-04-26 12:29:36", updated_at: "2021-04-26 12:29:59", optional_subset: true, published: false>]>

briri commented 2 years ago

The optional_subset flag is a way to identify that the GuidanceGroup is a child of another GuidanceGroup. It is a poor implementation though and has always been problematic because its not a true association.

There must always be a GuidanceGroup with optional_subset = false to act as the "parent" so that it can be displayed as:

   Some University
      |
      L___> Some school at the University

I'm guessing that in these scenarios there is not GuidanceGroup with an optional_subset of false. The user can change this value when editing the record. Another issue could be if there were multiple GuidanceGroups with the optional_subset of false ... which one is the correct parent?

For the short term, you could try updating the logic on that ensure method to allow for either optional_subset value. Not sure how it will properly render on the view though. Or, force that value to false in the case where there is only one GuidanceGroup.

Longer term, it would be a good idea to setup a true self referential relationship on that table. It might also make sense at this point to optionally allow those child guidance groups to be associated with departments.

johnpinto1 commented 2 years ago

Cheers @briri for comment. Might have time to look at next week.

nicolasfranck commented 2 years ago

maybe that filter, https://github.com/DMPRoadmap/roadmap/blob/master/app/controllers/guidances_controller.rb#L143, should not filter on optional_subset: false, as that incorrectly returns false, and assumes that the default guidance group doesn't exist yet, and tries to make one (which fails on validation). And yes, you could force that default group to optional_subset: false if it was set incorrectly..