Extract major domain concerns for application composability

[garethrees]

The application's code quality and development flexibility could be greatly improved by making it more composable. There are quite a few useful areas that could be generalised. This issue is just a kicking off point and way to group the similar work.

There are obviously more extractions we can make across the codebase – Classifiable, Trackable, Citable, etc – but the list below represents the major generalisations that can be widely shared and are the most important for adding new behaviour quickly.

Each of these concerns warrant their own issue, but at a high level I will articulate them here.

Reportable

Instead of marking records as attention_requested, and having to repeat all the reporting functionality across models, we should have a Report that handles all the behaviour associated with a report – alerting, closing, etc. We then make content Reportable by including a concern and creating a report against the reported record.

Commentable

Comments should be much more general purpose. There's the current in-public use, but we'd like to bring more admin discussion into the app. For example, I can imagine a PublicBodyChangeRequest having comments associated with it as part of internal discussion as to whether to accept the request. All of these are essentially comments, but right now a Comment is too tied to InfoRequest to be useful for anything else.

We'll also want to include Prominenceable to comments at some point, rather than the current "hidden" mechanism.

Prominenceable

We use prominence in several places, and have made a good start at extracting shared behavior into MessageProminence, but right now it doesn't feel easy to add prominence to other models – most notably FoiAttachment and Comment. Prominenceable shouldn't just be about messages.

It's probably not appropriate to back Prominenceable by a table in the same way as Reportable, but perhaps we want to take an approach similar to the has_tag_string or acts_as_xapian macros which allow the configuration of the appropriate prominence values.

Reindexable

This is a tiny piece of the code, but is core to how Alaveteli handles changes to records. We've got a hotchpotch of mechanisms for this right now:

OutgoingMessage
  after_update :xapian_reindex_after_update

IncomingMessage
  after_destroy :update_request
  after_update :update_request

Comment
  after_save :reindex_request_events

# And so on…

After we add Prominenceable to FoiAttachment, for example, which of these methods do we use? It would be better to just include Reindexable.

Taggable

We already have a has_tag_string macro and Taggable concern, but the underlying code is pretty confusing, it's not a polymorphic model, and there's probably a bit of cleanup in how we assign and use tags throughout the rest of the application.

Eventable

We currently have InfoRequestEvent, but it's jammed with stuff that isn't really about the InfoRequest itself. I feel we should have a single Event class, where any model can be Eventable – users creating pro subscriptions, bodies being updated, projects being joined. We can use Rails' new delegated_type to hand off much of the event-specific complexity to independent classes.

Approvable

We want to enable more "admin" work to be done by users, but with mechanisms to check that the work done is correct. In some cases this will be trusting by default and allowing a Report to be made, and in others we'll approve changes, like with PublicBodyChangeRequest. It feels like we should extract the approval mechanism to be more general purpose. The Approvable can contain the specific details of the action, whereas the Approval can handle whether to accept or reject with or without a reply, and create a record of the approver who made the decision.

Notable

A Note is slightly different to a Comment, I think. A Comment is part of the chain of events that happens on a Commentable. It's currently within the FOI request workflow, but equally could be part of a discussion on an Approval. A Note is more like a single item added to a larger whole – the whole request, or the whole body. It may receive several updates, rather than Comments which form a chain of correspondence and do not get edited.

The extraction to make initially is to extract PublicBody#notes (#4478) to a Note model, but then we want to be able to add a Note to a Tag, or add a Note to an InfoRequest.

Suggestible

Create a generic component to enable users to make suggestions in public that get reviewed/upvoted/downvoted by other users before a final admin approval to enact – enable higher level of decision-making by users.

[gbp]

I would add a sluggable concern to the above list.

PublicBody, InfoRequest and Users all currently have this baked into their models which could be extracted out

[gbp]

Re taggable, since 1b01522 the association is now polymorphic.

[garethrees]

We now want to version at least two classes – PublicBody and Note(#7279 (comment)) – so maybe Versionable.

I'm not yet sure whether I think a "version" is its own thing, or just an "edit" Event.

[garethrees]

Would be good to have a much easier and clearer way to deal with presenting redacted content.

Rather than implementing the redaction mechanism on each class, we could add a Redacted which handles all this. We'd only then need to define the applicable_censor_rules and applicable_text_masks – if a default is not sufficient – for the redactable object. Here's a sketch.

class Redacted
  def initialize(redactable)
    @redactable = redactable
  end

  def method_missing(method_name, *args, &block)
    redact(redactable.public_send(method_name, args))
  end

  private

  def redact(content)
    apply_censor_rules(apply_text_masks(content))
  end

  def apply_text_masks(content)
    # This is a little more complex in reality as we also pass the content-type
    AlaveteliTextMasker.apply_masks(
      content,
      masks: redactable.applicable_text_masks
    )
  end

  def apply_censor_rules(content)
    # CensorRule#apply would need to know whether the argument is text or binary
    redactable.
      applicable_censor_rules.
      reduce(text) { |text, rule| rule.apply(text) }
  end
end

Redacted.new(@incoming_message).subject
Redacted.new(@outgoing_message).body
Redacted.new(@info_request).title

[gbp]

I think we could have a Cacheable concern with some of the changes in #7893