Skip to content
5 changes: 5 additions & 0 deletions app/components/admin/editions/show/preview_component.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,11 @@
<p class="govuk-body">
<%= preview_link(primary_locale_link_text, local_edition.public_url(draft: true)) %>
</p>
<% if local_edition.is_a?(StandardEdition) && invalid_tab_forms&.any? %>
<p class="govuk-hint">
This preview may be out of date. Kindly fix the errors in the invalid tabs before the preview reflects your latest changes.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd move this above the preview link (above line 11) rather than having it sit between the main and the translated preview links.

I'd also get Sally to suggest a line of content. 🙏

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

And a view test to ensure this message appears when a tab is invalid. (So probably worth pulling this whole thing into its own commit)

</p>
<% end %>

<% if available_in_multiple_languages %>
<% translation_preview_links = local_edition.non_primary_translation_locales.map do |locale|
Expand Down
5 changes: 3 additions & 2 deletions app/components/admin/editions/show/preview_component.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,9 @@
class Admin::Editions::Show::PreviewComponent < ViewComponent::Base
include Admin::UrlOptionsHelper

def initialize(edition:)
def initialize(edition:, invalid_tab_forms: [])
@edition = edition
@invalid_tab_forms = invalid_tab_forms
end

def render?
Expand All @@ -13,7 +14,7 @@ def render?

private

attr_reader :edition
attr_reader :edition, :invalid_tab_forms

def versioning_completed
@versioning_completed ||= edition.versioning_completed?
Expand Down
36 changes: 34 additions & 2 deletions app/controllers/admin/standard_editions_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,26 @@ def apply_change_type
end

def update
@edition.current_tab_context = @current_tab_context
super
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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?

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

(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 🤔 )

@edition.assign_attributes(edition_params)

if @current_tab_context.present?
tab_form = StandardEdition::TabForm.new(@edition, @current_tab_context)

if tab_form.valid?
@edition.save_as(current_user, validate: false)
redirect_to redirect_param(fallback: show_or_edit_path), saved_confirmation_notice
else
apply_tab_errors_to_edition(tab_form)
build_edition_dependencies
fetch_version_and_remark_trails
construct_similar_slug_warning_error
render :edit
Comment on lines +51 to +54
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I did wonder if it's worth pulling out where this is called in EditionsController, into its own protected method, so that we can refer to that same method here rather than duplicate it?

end
else
# non-tabbed forms in standard edition, or non-tab pages like translations
@edition.current_tab_context = nil
super
end
end

def features
Expand All @@ -65,6 +83,15 @@ def features
render :features
end

def show
super
type_instance = @edition.type_instance
@invalid_tab_forms = type_instance.form_keys.filter_map do |tab_key|
tab_form = StandardEdition::TabForm.new(@edition, tab_key)
{ tab_key:, label: type_instance.form(tab_key)["label"] || tab_key.humanize } unless tab_form.valid?
end
end

private

def edition_class
Expand All @@ -89,6 +116,11 @@ def show_or_edit_path
end
end

def apply_tab_errors_to_edition(tab_form)
@edition.errors.clear
tab_form.errors.each { |e| @edition.errors.add(e.attribute, e.message) }
end

def render_not_found
render "admin/errors/not_found", status: :not_found
end
Expand Down
4 changes: 2 additions & 2 deletions app/models/concerns/edition/workflow.rb
Original file line number Diff line number Diff line change
Expand Up @@ -100,8 +100,8 @@ def pre_publication?
Edition::PRE_PUBLICATION_STATES.include?(state.to_s)
end

def save_as(user)
if save
def save_as(user, options = {})
if save(**options)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd expect a corresponding unit test here

edition_authors.create!(user:)
recent_edition_openings.where(editor_id: user).delete_all
end
Expand Down
65 changes: 65 additions & 0 deletions app/models/concerns/standard_edition/tab_form.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
class StandardEdition::TabForm
include ActiveModel::Model

attr_reader :edition, :tab_key

validate :validate_block_content
validate :validate_edition_fields, if: -> { edition_attribute_keys.any? }
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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


ADDITIONAL_DEFAULT_TAB_FIELDS = %w[title summary].freeze

def initialize(edition, tab_key)
@edition = edition
@tab_key = tab_key
end

private

def form_config
@form_config ||= edition.type_instance.form(tab_key).tap do |config|
raise ArgumentError, "Unknown tab key '#{tab_key}'" unless config
end
end

# Fields whose data lives inside block_content (e.g. "body", "social_media_links")
def block_content_field_keys
(form_config["fields"] || {}).filter_map do |_key, field|
attr_path = Array(field["attribute_path"])
attr_path.last if attr_path.first == "block_content"
end
end

# Fields whose data lives directly on the edition (e.g. "lead_organisation_ids")
def edition_attribute_keys
keys = (form_config["fields"] || {}).filter_map do |_key, field|
attr_path = Array(field["attribute_path"])
attr_path.first unless attr_path.first == "block_content"
end

tab_key == edition.default_tab ? keys + ADDITIONAL_DEFAULT_TAB_FIELDS : keys # Need to move these fields into document form configuration
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

end

def scoped_block_content
schema = edition.type_instance.schema_for_fields(block_content_field_keys)
StandardEdition::BlockContent.new(schema).tap do |bc|
bc.assign_attributes(edition[:block_content] || {})
end
end

def validate_block_content
block_content = scoped_block_content
return if block_content.valid?(validation_context)

block_content.errors.each { |error| errors.import(error, attribute: error.attribute.to_s) }
end

def validate_edition_fields
edition.current_tab_context = tab_key
edition.valid?(validation_context)
edition.current_tab_context = nil

edition_attribute_keys.each do |attr|
edition.errors.where(attr.to_sym).each { |error| errors.import(error, attribute: error.attribute.to_s) }
end
end
end
4 changes: 4 additions & 0 deletions app/models/configurable_document_type.rb
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,10 @@ def form(key = nil)
end
end

def form_keys
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Add corresponding unit test please

@forms.keys
end

def presenter(key)
@presenters[key]
end
Expand Down
20 changes: 17 additions & 3 deletions app/models/standard_edition.rb
Original file line number Diff line number Diff line change
Expand Up @@ -115,15 +115,18 @@ def group
end

def organisation_association_enabled?
field_paths.include?(ConfigurableContentBlocks::Path.new("lead_organisation_ids"))
current_tab_context_includes_field?("lead_organisation_ids") &&
field_paths.include?(ConfigurableContentBlocks::Path.new("lead_organisation_ids"))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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:

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") }

end

def worldwide_organisation_association_required?
required_field_paths.include?(ConfigurableContentBlocks::Path.new("worldwide_organisation_document_ids"))
current_tab_context_includes_field?("worldwide_organisation_document_ids") &&
required_field_paths.include?(ConfigurableContentBlocks::Path.new("worldwide_organisation_document_ids"))
end

def world_location_association_required?
required_field_paths.include?(ConfigurableContentBlocks::Path.new("world_location_ids"))
current_tab_context_includes_field?("world_location_ids") &&
required_field_paths.include?(ConfigurableContentBlocks::Path.new("world_location_ids"))
end

def is_in_valid_state_for_type_conversion?
Expand Down Expand Up @@ -187,4 +190,15 @@ def required_field_paths
def string_for_slug
title if primary_locale.to_sym == translation.locale
end

def current_tab_context_includes_field?(attribute_name)
return true if current_tab_context.blank? # assume we are checking against the entire edition

form = type_instance.form(current_tab_context)
return true if form.nil?

(form["fields"] || {}).any? do |_key, field|
Array(field["attribute_path"]).include?(attribute_name)
end
end
end
3 changes: 1 addition & 2 deletions app/views/admin/editions/show/_main.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,7 @@
</section>

<%= render partial: "admin/editions/show/main_notices", locals: { edition: @edition } %>

<%= render Admin::Editions::Show::PreviewComponent.new(edition: @edition) %>
<%= render Admin::Editions::Show::PreviewComponent.new(edition: @edition, invalid_tab_forms: @invalid_tab_forms) %>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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


<% if @edition.change_note_required? %>
<section class="app-view-summary__section">
Expand Down
13 changes: 12 additions & 1 deletion app/views/admin/editions/show/_sidebar_notices.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,17 @@

<div class="app-view-summary__sidebar-notices">
<% unless @edition.valid?(:publish) %>
<% invalid_items = if @edition.is_a?(StandardEdition) && @invalid_tab_forms&.any?
@invalid_tab_forms.map do |tab|
link_to(
"#{tab[:label]} tab is invalid",
edit_admin_standard_edition_path(@edition, current_tab: tab[:tab_key]),
class: "govuk-link",
)
end
else
@edition.errors.map(&:full_message)
end %>
<%= render "components/inset_prompt", {
data_attributes: {
module: "ga4-auto-tracker",
Expand All @@ -18,7 +29,7 @@
title: "This edition is invalid",
description: render("govuk_publishing_components/components/list", {
visible_counters: true,
items: @edition.errors.map(&:full_message),
items: invalid_items,
}),
error: true,
} %>
Expand Down
Loading