Validate each document tab form separately [WHIT-3214]

[eYinka]

What

Move the edition validation off of the edition model on to the configurable form object, and validate each form independently. Make sure that all forms are validated before the edition can be published.

In the scope of this implementation:

• Errors for the form are rendered to the user at the top of the page and next to the invalid form field, with a link from the top of the page to the invalid form
• Any edition tab’s form can be saved regardless of the validity of the other tabs
• All edition forms are validated before the “Publish” button is displayed to the user, and any errors are displayed to the user on the summary page.
• Only a globally valid edition is updated in Publishing API
• Users can see a hint text letting them know draft previews may be out of date if an edition has invalid tabs

Why

• Accelerates future development of configurable document types with a more appropriate abstraction
• Decouples form validation and attributes from the Edition model

Visual Changes

Testing

Experiencing the full benefit of this work is tricky, because you will almost never have a saved multi-tab edition in bad state unless you manually manipulate the data.
Currently, the only way to access dynamic tabs is by saving the document tab first, which means the draft is fully valid before any subsequent updates can be made. Therefore, it's not inherently clear that we are scoping validations per tab.

For anyone that wants to give it a go locally, an easy way is to configure a document type (like topical event) with "send_change_history": true, then:

• Redraft a published topical event - this should surface the change history form on the document tab.
• Without filling the change history form, try to save another tab (i.e Social media links) with invalid data. You should only see errors related to social media tab and nothing about the invalid change history should be displayed. Also you will be able to see a summary of the invalid tabs on the edition summary page.
• Alternatively, you can grab a draft edition and manipulate the data by doing the following:

# Grab the edition
edition = Edition.find(INSERT_EDITION_ID)

# Manipulate the block content by setting the body to nil and storing an invalid social media link
new_content = edition.translation[:block_content].merge("body" => nil, "social_media_links" => [{ "url" => nil, "social_media_service_name" => "Facebook" }])

# Persist the data
edition.translation.update_column(:block_content, new_content)

Future Improvements:

• When all document types have been migrated to using config-driven tab forms, we should be able to remove validate_block_content and scoped_block_content from has_block_content.rb.
  TabForm will then be the single point for handling all block_content validation. For now, it's fine to keep the validation in both places because not all tabs are currently fully config-driven, and edition.valid? is called in several places that don't go through TabForm.
• Add title and summary to the documents schema of all config driven document types
• In the spirit of maintaining consistency, enforce adding a label to each form in document type configurations.
• Maintain a single way to access the documents tab. Currently, it is accessible via /standard-editions/:id/edit and /standard-editions/:id/edit?current_tab=documents. The latter is necessary for validating only the documents tab while the former will assume full edition validation.

JIRA

---

⚠️ This repo is Continuously Deployed: make sure you follow the guidance ⚠️

This application is owned by the Whitehall Experience team. Please let us know in #govuk-whitehall-experience-tech when you raise any PRs.

Follow these steps if you are doing a Rails upgrade.

[ChrisBAshton]

Well done for working through such a tricky problem ⭐

Whilst what you have works, it does spread the tab-awareness throughout some quite key Whitehall publishing workflows. I'd hope that given we have a dedicated StandardEditionsController, we could be a bit more precise about keeping tab awareness 'contained' and not having to reference low-level StandardEdition concepts from high level Edition Services etc.

It may be easier said than done , but I think it's worth spending a few days considering how self-contained we can make the tab validation, and also how self-contained we can make the 'associations' validation (rather than changing the meaning of the associations helper methods in the first commit). 🙏 Give me a shout if you'd like to talk any of this through.

[ChrisBAshton]

I'm a bit concerned this is in the wrong place in the stack. We want to be able to ask "for this type of StandardEdition, are organisations enabled?". That's a business logic / model decision. Whether or not to enforce validation on that association is more of a controller-level one, I'd say.

Making the changes here risks introducing bugs in, say, the Edition Access Limited controller's changed? method, or risks us not calling the build_default_organisation method on the Edition Controller, or there may be other places too where we rely on this method call being a true reflection of what is/isn't enabled on a document type.

I think really this sort of branching should move to:

whitehall/app/models/concerns/edition/organisations.rb

Lines 16 to 17 in 5521aee

	validate :at_least_one_lead_organisation, if: :organisation_association_enabled?
	validate :no_duplication_of_organisations, if: :organisation_association_enabled?

Something like:

    validate :at_least_one_lead_organisation, if: -> { organisation_association_enabled? && current_tab_context_includes_field?("lead_organisation_ids") }

[ChrisBAshton]

I'd move the conditional inside validate_edition_fields as an early return, instead of here.

[ChrisBAshton]

I think we can drop the const as it's only used here.
Can also make it a bit more optimised for deletion as below:

Suggested change

	tab_key == edition.default_tab ? keys + ADDITIONAL_DEFAULT_TAB_FIELDS : keys # Need to move these fields into document form configuration
	# TODO: Move these fields into document form configuration
	# so that we don't have to inject them here
	if tab_key == edition.default_tab
	keys << %w[title summary]
	end
	keys

[ChrisBAshton]

I'd expect a corresponding unit test here

[ChrisBAshton]

We're moving away from calling super on every update, to only calling super when saving a non-tabbed form. Have we explored the downsides of that?

whitehall/app/controllers/admin/editions_controller.rb

Lines 124 to 148 in 52e5953

	def update
	@edition.assign_attributes(edition_params)
	if updater.can_perform? && @edition.save_as(current_user)
	updater.perform!
	if @edition.link_check_report
	LinkCheckerApiService.check_links(@edition, admin_link_checker_api_callback_url)
	end
	redirect_to redirect_param(fallback: show_or_edit_path), saved_confirmation_notice
	else
	flash.now[:alert] = updater.failure_reason
	build_edition_dependencies
	fetch_version_and_remark_trails
	construct_similar_slug_warning_error
	render :edit
	end
	rescue ActiveRecord::StaleObjectError
	flash.now[:alert] = "This document has been saved since you opened it"
	@conflicting_edition = Edition.find(params[:id])
	@edition.lock_version = @conflicting_edition.lock_version
	build_edition_dependencies
	render :edit
	end

Calling super means calling updater.perform!, rather than @edition.save_as - does the DraftEditionUpdater service do something important we'd lose otherwise?

Also currently super means a fresh LinkCheckReport is generated, which we'd currently be losing by not calling that.

[ChrisBAshton]

(it may be that we want to call super when updating the Document tab, but otherwise call our own logic if saving any other tab 🤔 )

[ChrisBAshton]

Hmmm this extra param is being passed, but doesn't appear to be being used anywhere?

[ChrisBAshton]

Suggested change

	[{ label: "Document", href: base_url, current: on_dynamic_tab && on_default_tab }].tap do |nav_items|
	[{ label: "Document", href: base_url, current: on_default_tab }].tap do |nav_items|

[ChrisBAshton]

Is this not already caught by the elsif !edition.valid? check in failure_reason?

If not, failure_reason feels like the right place to capture a StandardEdition specific check - rather than introducing a change here.

But I'm also confused because in my earlier comment, it looks as though we never call the DraftEditionUpdater service with the current changes to the StandardEditionController 🤔
And actually maybe that's a better approach. By the time we get to the DraftEditionUpdater, we perhaps should already be confident that every tab's individual validation issues have been fixed.

[ChrisBAshton]

If we do need to expose the tab form validation at a higher level than the StandardEditionController, can we at least wrap it in a simpler helper method so we're not having to reference form keys etc? Something like:

Suggested change

	edition.type_instance.form_keys.all? do |tab_key|
	edition.valid?(:on_all_tabs)

[ChrisBAshton]

Yeah I'm not keen that we've overridden the meaning of edition.valid? or edition.valid?(:publish) and now have to update the rest of Whitehall accordingly. I think one of the AC's for this work, in hindsight, should be that calling edition.valid?(:publish) checks for validation errors across all tabs. That way we don't have to update any of these higher level workflows.

And then at the lower StandardEdition level, we just stop calling edition.valid?(:publish) and start being tab-aware instead, e.g.

edition.validation_tab = tab
edition.valid?(:tabbed)