Initial spike for object store #9112

pezholio · 2024-06-04T11:13:39Z

This follows on from #9063 to add the initial scaffolding for an object store in Whitehall, which will allow us to manage reusable blocks of content.

An item for an object store is an editionable object which inherits from the root Edition model. We did look at defining a smaller subset of code that would make a model editionable, but this turned out to be too much of a rabbit hole.

Items can have a shape which is defined in config, for now we're having one content type, an email address, but going forward, we will want many different types of object. Ideally, we'd also like these content types to be defined by non-developers.

I've added a very basic create/edit flow for items too (which will need iteration, both for design and functionality), but this gives us a good idea of how this will all fit together, as well as highlighting the changes that need to be made upstream for us to send content to the Publishing API.

tahb

Thanks for breaking down this change into all the steps 👍

As someone who's just joined without any background of the plan I've got quite a few thoughts and given this is a foundational piece they feel like good conversations to have now, rather than later, so forgive the number of them please!

Naming: I notice we use "item" and "content" in different places. I think "item" may be too generic and so "content_item" and "content_item_type" might provide more clarity.

tahb · 2024-06-04T13:53:52Z

db/migrate/20240529133137_add_details_to_edition.rb

@@ -0,0 +1,5 @@
+class AddDetailsToEdition < ActiveRecord::Migration[7.1]
+  def change
+    add_column :editions, :details, :json


What's the default value for this, is it nil?

If so, do we need to set it to default: {} and migrate existing records to have an empty {}?

def up add_column :editions, :details, :json, default: {} Editions.update_all(details: {}) end def down remove_column :editions, :details end

tahb · 2024-06-04T13:58:00Z

db/migrate/20240529133137_add_details_to_edition.rb

+  def change
+    add_column :editions, :details, :json
+  end
+end


Editions are a polymorpic model, so there are a lot of fields that are defined in the database that are not required everywhere. To prevent having to add columns that aren’t relevant everywhere, we add a new JSON column that will contain generic detail about our Content Items

It sounds like we are adding in a second way to do things, the previous way was to add new columns even if they weren't used by all consumers of Editions. Is this new details field part of a wider strategy to address this by encouraging other teams to also make use of this? It feels like an ADR would help capture the future vision for this and why we're breaking convention.

tahb · 2024-06-04T14:21:01Z

lib/object_store.rb

+    configuration.fields[item_type] || raise(UnknownItemType, item_type)
+  end
+
+  def self.fields_for_item_type(item_type)


It feels like there could be two small objects here: item_type and field as a separate concept from "field_definition" (the config representation).

We already want to send messages to them such as item_type.required? and field.required?, and I imagine this behaviour is going to extend somewhat?

That's a good call that, still getting my head back into a Ruby-ish way of thinking!

tahb · 2024-06-04T14:24:51Z

lib/object_store.rb

+  end
+
+  def self.configure
+    yield(configuration)


I wonder if we should validate the structure of is configuration so we can make certain assumptions about it, ensuring a certain structure and making sure that we don't accidentally break it.

Something like Psych to load the yml and check the structure? We should probably figure out a way to put that into the CI tests too.

tahb · 2024-06-04T14:31:27Z

test/unit/lib/object_store_test.rb

+require "test_helper"
+
+class ObjectStoreTest < ActiveSupport::TestCase
+  setup do


Given we are making guards against missing fields I think we ought to test the sad path here too, covering happens when this config file is missing keys or contains unexpected keys or values.

tahb · 2024-06-04T14:33:23Z

app/models/object_store/item.rb

@@ -0,0 +1,33 @@
+module ObjectStore
+  class Item < Edition


I'm not sure yet what we need from Edition but given we said earlier we don't care about a lot of stuff it adds, do we need inheritance here or should we use composition?

module LiteEdition def self.included(base) base.class_eval do store_accessor :details, :field1, :field2 … validates :field1, presence: true … end end end class Item < ApplicationRecord include LiteEdition end

I have been considering what a more composition based approach might look like in https://github.com/alphagov/whitehall/blob/edition-refactoring/docs/adr/0003-introduce-reusable-content-items.md, which might be of interest. A LiteEdition module might be an interesting step in that direction, to see how much behaviour we can lift off of the edition model.

tahb · 2024-06-04T14:41:56Z

app/models/object_store/item.rb

+
+  private
+
+    def add_field_accessors


If we were to reach for a field object we could decouple ourselves from ObjectStore a little with something like:

def fields @fields ||= ObjectStore.fields_for_item_type(item_type) end def add_field_accessors @fields.each do |field| singleton_class.class_eval { store_accessor :details, field.name } if field.required? singleton_class.class_eval { validates fields.name, presence: true } end end rescue UnknownItemType # Ignored end

tahb · 2024-06-04T14:54:14Z

test/integration/object_store/create_email_test.rb

+require "test_helper"
+require "capybara/rails"
+
+class CreateEmailTest < ActionDispatch::IntegrationTest


I read this and mistook it for creating/sending an email. Maybe worth renaming to create_email_content_item_test.rb or create_email_address_test.rb.

ryanb-gds

I think we want to be really, really confident that we want to start storing data using JSON before we go down this path. I'd echo @tahb concerns about breaking with the convention of using database columns lightly. My suspicion is that as we start adding more options to the configuration, such as validation, multiple value fields etc., which are features people always want in CMS, we'll think it might have been easier to define the types in code after all and leverage Active Record.

If we do want to stick to JSON and a config file, then should we just be using the schema from Publishing API instead? I don't much like the idea of having to keep a config file in Whitehall in sync with a config file in Publishing API.

ryanb-gds · 2024-06-04T16:14:06Z

config/object_store/fields.yml

+email_address:
+  properties:
+    email_address:
+      type: string


Is it going to be possible to validate that this input is, in fact, a valid email address etc.? Or do, say, relative validation against another field value?

Yeah, it's definitely worth considering, but I wanted to get the pipework in first

ryanb-gds · 2024-06-04T16:34:59Z

app/models/object_store/item.rb

@@ -0,0 +1,33 @@
+module ObjectStore
+  class Item < Edition


I have been considering what a more composition based approach might look like in https://github.com/alphagov/whitehall/blob/edition-refactoring/docs/adr/0003-introduce-reusable-content-items.md, which might be of interest. A LiteEdition module might be an interesting step in that direction, to see how much behaviour we can lift off of the edition model.

pezholio · 2024-06-05T07:54:23Z

@ryanb-gds Thanks for the feedback I did give the idea of a "Lite Edition" a go in the previous PR, but I was concerned that it was going to eat up too much time and risk impacting the wider team too much, as there's a ton of moving parts in the concerns that make assumptions about the shape of the inheriting model.

Part of my thinking re: JSON columns was to give flexibility to create new content types on the fly. I did consider using the Publishing API schemas to define the types, but I was actually thinking about making the schemas on the Publishing API less strict, so an "Object Store Item" schema could, in effect accept any shape / size of object.

Would be good to have a bit more of a detailed chat about all of this, there's definitely some interesting stuff in that ADR we could adapt too.

At the moment, Editions are a polymorpic model, so there are a lot of fields that are defined in the database that are not required everywhere. To prevent having to add columns that aren’t relevant everywhere, we add a new JSON column that will contain generic detail about our Content Items. This also gives us flexibility to define new Content Item types without having to further overload the database, or create new models etc. It also gives us the flexibility to make creating new content types a non-developer task in future if we want / need to.

In the next commit we’ll be adding an `ObjectStore::Item` model, which will eventually have any number of fields. Here we add some Yaml config to define the fields for an `email_address` item type, and the fields that are required for it. We also add an `ObjectStore` configuration module, which allows us to store / pull configuration in a meaningful way, rather than pulling values out of hashes all over the code.

This inherits the edition model and sets a few defaults to override some Edition defaults. It also contains an `add_field_accessors` callback that reads the configuration for a given item_type and sets the relevant variables using [`store_accessor`][1]. Additionally, if the field is required, we set validation on the object too. [1]: https://api.rubyonrails.org/classes/ActiveRecord/Store.html

At the moment, Editions are a polymorpic model, so there are a lot of fields that are defined in the database that are not required everywhere. To prevent having to add columns that aren’t relevant everywhere, we add a new JSON column that will contain generic detail about our Content Items. This also gives us flexibility to define new Content Item types without having to further overload the database, or create new models etc. It also gives us the flexibility to make creating new content types a non-developer task in future if we want / need to.

This ensures that item_types only have values that are defined in the config, as well as ensures they cannot be changed once persisted.

This adds a new controller that inherits from the main `EditionsController` and overrides some of the default behaviour to allow our items to be edited/created. We also add a new edit/new view to override the main editions views which are already quite overloaded with logic. At the moment, creation and editing fails when sending data to the Publishing API, but this is a start.

While we’re working on this, let’s add a feature flag using Flipflop[1]. By default this will be turned on in development, but off in other environments until we’re ready to test/roll out. [1]: https://github.com/alphagov/whitehall/blob/main/docs/feature_flags.md

This allows us to use an `ObjectStore::Item` with minimal baggage from the Edition model.

This ensures we don’t repeat ourselves

tahb · 2024-06-17T16:33:18Z

@pezholio Great to see LiteEdition emerging 🎉

It's difficult for me to see and feel confident that we've drawn the lines of responsibility right.

Earlier when we spoke the list was something like this:

draft state
history of object instances
scheduling of publishing
2ii reviews
ability to force push
future: advanced ownership via roles to limit management and usage
more?

Are we the first to have the need for a LiteEdition? I'm thinking that if any part of the codebase has needed something similar in the past it might be an idea to compare their needs and review what we've got.

If we're setting the path and the concept is agreed on then I wonder if a next step could be to move this set of changes into a Whitehall fork so that we can take it forward at pace. I think it would be good to test this theory out and be able to tweak it without having to overly worry about all the considerations of merging it into main. Once we've proved LiteEdition works and we've got a reasonable boundary we could make a clearer proposition? I'm assuming we can do a meaningful test with a local set up that has all parts of the stack we need?

pezholio · 2024-06-27T14:18:29Z

Closing in favour of #9204

pezholio mentioned this pull request Jun 4, 2024

Begin modelling a Content Library within Whitehall #9063

Closed

pezholio force-pushed the initial-spike-for-object-store branch 2 times, most recently from 8a0ec2c to 773b3cd Compare June 4, 2024 11:52

tahb reviewed Jun 4, 2024

View reviewed changes

ryanb-gds reviewed Jun 4, 2024

View reviewed changes

pezholio force-pushed the initial-spike-for-object-store branch 5 times, most recently from dd8c8c7 to dd61d6a Compare June 5, 2024 15:03

pezholio added 7 commits June 17, 2024 11:06

Add validation for item_type

b508e07

This ensures that item_types only have values that are defined in the config, as well as ensures they cannot be changed once persisted.

pezholio force-pushed the initial-spike-for-object-store branch 3 times, most recently from c6a9db8 to 8736d9f Compare June 17, 2024 12:17

pezholio added 2 commits June 17, 2024 13:21

Add a LiteEdition concern

9fca40e

This allows us to use an `ObjectStore::Item` with minimal baggage from the Edition model.

Include LiteEdition in Edition model

863801e

This ensures we don’t repeat ourselves

pezholio force-pushed the initial-spike-for-object-store branch from 8736d9f to 863801e Compare June 17, 2024 12:22

pezholio mentioned this pull request Jun 19, 2024

SPIKE: creating an Object Store for Content Blocks #9180

Closed

tahb mentioned this pull request Jun 26, 2024

Introduce foundations for the Content Object Store with a Rails Engine #9204

Merged

pezholio closed this Jun 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial spike for object store #9112

Initial spike for object store #9112

pezholio commented Jun 4, 2024

tahb left a comment •

edited

Loading

tahb Jun 4, 2024

tahb Jun 4, 2024

tahb Jun 4, 2024

pezholio Jun 5, 2024

tahb Jun 4, 2024

tahb Jun 4, 2024

tahb Jun 4, 2024

ryanb-gds Jun 4, 2024

tahb Jun 4, 2024

tahb Jun 4, 2024

ryanb-gds left a comment

ryanb-gds Jun 4, 2024

pezholio Jun 5, 2024

ryanb-gds Jun 4, 2024

pezholio commented Jun 5, 2024

tahb commented Jun 17, 2024 •

edited

Loading

pezholio commented Jun 27, 2024

Initial spike for object store #9112

Initial spike for object store #9112

Conversation

pezholio commented Jun 4, 2024

tahb left a comment • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanb-gds left a 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

pezholio commented Jun 5, 2024

tahb commented Jun 17, 2024 • edited Loading

pezholio commented Jun 27, 2024

tahb left a comment •

edited

Loading

tahb commented Jun 17, 2024 •

edited

Loading