Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[#7811] Generalised category system #8023

Merged
merged 8 commits into from
Nov 30, 2023
Merged

[#7811] Generalised category system #8023

merged 8 commits into from
Nov 30, 2023

Conversation

gbp
Copy link
Member

@gbp gbp commented Nov 22, 2023

Relevant issue(s)

Part of #7811

What does this do?

Adds generalised category system, replacing the old PublicBodyCategory models.

Why was this needed?

So we can categorise requests.

Implementation notes

There is one minor changes to the old category feature. Due to how nested sets work categories can now only have one parent/heading.

This means:

  1. I didn't add to Category the uniqueness validation which is on PublicBodyCategory#category_tag.

  2. Some categories have been duplicated, for WDTK this means 27 categories with the same title as another will be created. I think this is a good compromise given the reduced code complexity and added flexibility of being able to reuse the new categories feature for InfoRequest easily.

@gbp gbp force-pushed the 7811-categories branch 3 times, most recently from c45a953 to cb345a3 Compare November 22, 2023 16:45
@garethrees
Copy link
Member

There is one minor changes to the old category feature. Due to how nested sets work categories can now only have one parent/heading… Some categories have been duplicated, for WDTK this means 27 categories with the same title as another will be created.

Could you paste a list of these?

@gbp
Copy link
Member Author

gbp commented Nov 22, 2023

Could you paste a list of these?

irb(main):002:0> PublicBodyCategory.all.select { _1.public_body_headings.count > 1 }.map(&:title).sort
=>
["Ambulance Services",
 "Approved regulators of legal services",
 "Areas of Outstanding Natural Beauty Conservation Boards",
 "Associations of Local Councils",
 "Dance companies",
 "Education authority",
 "Education regulators",
 "Financial regulators",
 "Groups of universities",
 "Health and Social Care Partnerships (Scotland)",
 "Health and social care regulators",
 "Health regulators",
 "Heraldic authorities",
 "Highways Operators",
 "Housing ALMOs",
 "Housing regulators",
 "Integrated Care Partnerships",
 "Integration Joint Boards",
 "Media regulators",
 "Military colleges",
 "Money laundering supervisory authorities",
 "Police regulators",
 "Telecommunications companies",
 "Theatres",
 "Transport regulators",
 "Utility regulators",
 "Veterans Advisory and Pensions Committees"]

@gbp gbp marked this pull request as ready for review November 23, 2023 09:57
@gbp gbp requested a review from garethrees November 23, 2023 09:57
spec/fixtures/categories.yml Outdated Show resolved Hide resolved
Copy link
Member

@garethrees garethrees left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good as discussed on call / demo.

Main thing is to make sure that we can add categories to many Taggable things in future. Seems like we should be able to push the currently hard-coded things referencing an explicit model into a concern:

# BEFORE:
Category.public_body_tree.each do |heading|

# AFTER: 
PublicBody.categories.each do |heading|

# BEFORE: In category
def self.public_body_root
  Category.roots.find_or_create_by(title: 'PublicBody')
end

def self.public_body_tree
  tree(public_body_root)
end


# AFTER: In categorisable
def self.category_root
  Category.roots.find_or_create_by(title: self.class.name)
end

def self.categories
  Category.tree(category_root)
end

app/models/category.rb Outdated Show resolved Hide resolved
lib/tasks/temp.rake Show resolved Hide resolved
@gbp
Copy link
Member Author

gbp commented Nov 29, 2023

@garethrees this has been reworked to address the concern we noticed the other day.

I have left the fixups so you can see what has been changed. But basically this is combination of the initial approach and the ideas we came up with when we discussed this.

Main difference is instead of switching to having a model_type column, I have left the PublicBody root category as the top-level parent. I noticed if the headings didn't have a parent then there would be no CategoryRelationship instances and no place to store position, so with a Category#model_type column we would also need a Category#root_position column which makes instance ordering more complex and would mean we needed separate parent and child category reordering controller actions. Leaving the root model category concept simplifies the model and controller actions.

@gbp gbp requested a review from garethrees November 30, 2023 08:41
@gbp gbp removed their assignment Nov 30, 2023
gbp added 5 commits November 30, 2023 11:32
This model uses nested sets model technique to store hierarchy and the
ordering of categories.

Adds the same associations from existing `PublicBodyCategory` model
which this model will be replacing to retain backwards compatibly for
now.

As `PublicBodyHeading` will be migrated into to `Category` instances we
have dropped the validation on the `description` and `category_tag`
attributes.
Migrate `PublicBodyCategory` and `PublicBodyHeading` to `Category`
retaining hierarchy and ordering.

`PublicBodyCategory` could be linked with more than one heading but
instances of new `Category` model can only have one parent. For these
instance we have to create duplicate categories.
Update rendering to show new `Category` instances.
Update rendering to show new `Category` instances.
gbp added 3 commits November 30, 2023 11:32
With the old routes now removed we can rename the new category routes to
match the old.
@gbp
Copy link
Member Author

gbp commented Nov 30, 2023

Reviewed and approved on a call together.

@gbp gbp merged commit 137f10f into develop Nov 30, 2023
7 checks passed
gbp added a commit that referenced this pull request Jan 9, 2024
Updated to use new category system introduced in #8023 over the older
`PublicBodyCategory` model which isn't being updated anymore.

Fixes #8081
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants