search

localgovdrupal / localgov_workflows

Default editorial workflow for LocalGov Drupal content.
GNU General Public License v2.0
0 stars 1 forks source link

Workflow: required revision log? #10

Closed finnlewis closed 2 years ago

finnlewis commented 3 years ago

@tom-steel raised the question of revision log.

Sometimes it would be good to ensure that the revision log is required.

If it is, the positioning of the revision log field is important: possibly move from the sidebar to the main content fields on the admin UX?

finnlewis commented 3 years ago

Passing this one to @tom-steel to refine.

finnlewis commented 3 years ago

Discussing this with @msayoung and @stephen-cox - hoping to bring into the next sprint.

Assumptions:

@tom-steel any thoughts

finnlewis commented 3 years ago

Just for the record, @davidsiddall confirmed this is also wanted at Waltham Forest

tom-steel commented 3 years ago

Discussing this with @msayoung and @stephen-cox - hoping to bring into the next sprint.

Assumptions:

  • Global configuration option on a form to enable 'require revision log' (default to off)
  • form-alter to set the field to required on node forms
  • maybe move the location of the field to somewhere obvious.

@tom-steel any thoughts

The history of form changes can provide valuable context to further iterations, and the impact of a change can have a major impact on a service, so recording notes for changes can be very valuable.

User stories When an editor needs to update a form They need to know any history relating to previous changes So that they understand any hidden context to a forms current function

When an editor needs to update a form They need to easily provide context and reason for the change So that future editors can understand how the form has evolved over time.

When an editor is asked why a change was made to a form They need to know when the form changed, and why So they can evidence the changes to those concerned.

Acceptance criteria: A reason/note for each revision change can be viewed at a glance. Adding a reason/note is part of the UX for saving and publishing revisions

andybroomfield commented 3 years ago

This could get annoying though if there are a few minor amends that needs to be made, so maybe a checkbox that says minor amends that can pre-fill the revision log message. (Or some pre filled options like fix typos etc.)

ekes commented 3 years ago

Am I correct that https://github.com/localgovdrupal/localgov_workflows/issues/10#issuecomment-922806804 is about forms (webforms); and https://github.com/localgovdrupal/localgov_workflows/issues/10#issuecomment-896731376 is about nodes?

tom-steel commented 3 years ago

This could get annoying though if there are a few minor amends that needs to be made, so maybe a checkbox that says minor amends that can pre-fill the revision log message. (Or some pre filled options like fix typos etc.)

I agree, making it mandatory isn't crucial i think, what's most important is that the feature is positioned as part of the "save" area so it's hard to ignore. Currently it's in the sidebar which is easily out of sight when you save an update.

tom-steel commented 3 years ago

Am I correct that #10 (comment) is about forms (webforms); and #10 (comment) is about nodes?

Both are about changes to webforms.

ekes commented 3 years ago

form-alter to set the field to required on node forms

finnlewis commented 3 years ago

@tom-steel as far as I know we've focussed our discussion on node content entities as part of the general content workflows. We don't yet have webforms in the distribution so we might want to separate out the requirements for standard node content and webforms into different issues / feature requests.

ekes commented 3 years ago

So to separate issues out; and look to the future:-

I think it probably would be possible, I'll investigate, to set a matrix or similar of entity types and bundles to alter the form on to require something in the revision log field.

The question of how diff's between versions, and the log, is displayed might however vary per entity type; but that should be handled with extending log and diff if required?

willguv commented 3 years ago

This could get annoying though if there are a few minor amends that needs to be made, so maybe a checkbox that says minor amends that can pre-fill the revision log message. (Or some pre filled options like fix typos etc.)

I agree, making it mandatory isn't crucial i think, what's most important is that the feature is positioned as part of the "save" area so it's hard to ignore. Currently it's in the sidebar which is easily out of sight when you save an update.

In the GDS Whitehall publisher there was the idea of a "major change"

Minor typo changes could be published largely without checking

Major changes were flagged by a content designer ticking a "major change" check box which opened up a mandatory revision notes field where the change had to be described

It relied on content designers taking this step, but they largely did

ekes commented 3 years ago

Research of what others have done before section:-

ekes commented 3 years ago

Research on how it works (technical):-

The revision system is generic for content entities, and the log field name is defined in the entity annotation, it's a base field, so for any content entity bundle you can set an override for that field to make it compulsory:-

// Content entity type and bundle to operate on.
$entity_type_id = 'node'; 
$bundle_name = 'localgov_services_page'; 

// We want the field and type managers.
$entity_field_manager = \Drupal::service('entity_field.manager');                                         
assert($entity_field_manager instanceof EntityFieldManagerInterface);                                     
$entity_type_manager = \Drupal::service('entity_type.manager');                                           
assert($entity_type_manager instanceof EntityTypeManagerInterface);

// Only content entities.
$entity_type = $entity_type_manager->getDefinition($entity_type_id);
if (!$entity_type instanceof ContentEntityType) { 
  return 'Does not support revisions'; 
}
$base_fields = $entity_field_manager->getBaseFieldDefinitions($entity_type_id);

// What field is used for the log message is defined in the Entity annotation.
$revision_log_field_name = $entity_type->getRevisionMetadataKey('revision_log_message');
$revision_log_field = $base_fields[$revision_log_field_name];
// ::getConfig returns the BaseFieldOverride for the bundle.                                              
$revision_log_field_configuration = $revision_log_field->getConfig($bundle_name); 
$revision_log_field_configuration->setRequired(TRUE);
$revision_log_field_configuration->save();

Of the modules mentioned above only the (acquia) revision log default actually does not assume the revisions log field name. It's working in pre_save not on the form. The others hard code the field name which is different in core even between nodes and media.

Anyway so creating those overrides is possible for any content entity. However, configuration of things like the workflow section on the configuration of a Node Type isn't standardized eg: https://git.drupalcode.org/project/drupal/-/blob/f680e8ea6c5137b8d5c1a9e942f2d1f94828476b/core/modules/node/src/NodeTypeForm.php#L147

So I suggest we set the base field override correctly, but we either:

I'd tend toward the former. I'd really expect the config to be underneath the 'Create new revision' checkbox on the Node Type form. This does not address: that the log message is required before previewing (a minor issue I think, and we tend not to have preview on); that there are no defaults or opt-out or so...

finnlewis commented 2 years ago

@ekes is this done with https://github.com/localgovdrupal/localgov_workflows/pull/21?

ekes commented 2 years ago

21 covers the basic requirement of requiring a log message on content entities, configurable per-content type.

I'd say if there are further wishes should make new issues.