-
Notifications
You must be signed in to change notification settings - Fork 198
Implement StandardEdition Parent/Child relationships with semi-independent publishing workflows [WHIT-3341] #11431
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Draft
ChrisBAshton
wants to merge
22
commits into
main
Choose a base branch
from
spike-independently-editable-standardedition-children
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from all commits
Commits
Show all changes
22 commits
Select commit
Hold shift + click to select a range
78bda4a
Create table to track relationship between Parent and Child docs
ChrisBAshton c5ef7b8
Define ParentChildRelationship class
ChrisBAshton d9a3685
Create ChildDocument and ParentDocument concerns on StandardEdition
ChrisBAshton 697631d
Add process_associations_after_save on Parent edition creation
ChrisBAshton 7d099aa
Add 'allowed_child_document_types_of' method
ChrisBAshton 40c1b23
Add route / controller / view for choosing child document type
ChrisBAshton fb34782
Add association between parent/child doc on child doc creation
ChrisBAshton 1fe0ca2
Add Topical Event About Page config (and reference it from parent)
ChrisBAshton 38a2628
Add `allows_child_documents?` method, driven by config
ChrisBAshton 5448676
Send 'parent' link when child document type is configured to do so
ChrisBAshton 18a12ee
Enforce either base_path_prefix OR is_child_document property
ChrisBAshton fb6d226
Infer child document's base_path_prefix from the parent doc
ChrisBAshton d5750a3
Add child document creation journey end-to-end
ChrisBAshton bd1b96d
Rule 1: New child drafts
ChrisBAshton 1fee4c0
Add 'new_child_documents' helper method
ChrisBAshton 49c0992
Ensure deleting a child draft removes it from parent
ChrisBAshton 3b7a3b4
Rule 2: Prevent orphaned children
ChrisBAshton 07be10a
Rule 3: Child publishability
ChrisBAshton 3c0a9d8
Rule 4: Parent visibility ceiling
ChrisBAshton 5b242f7
Add validation to ensure child can't be linked to multiple parents
ChrisBAshton f474298
Move Parent/Child relationship logic to StandardEditionsController
ChrisBAshton 62b6b42
Rename can_be_published? to parent_allows_publishing?
ChrisBAshton File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| class Admin::ChildDocumentsController < Admin::EditionsController | ||
| rescue_from ConfigurableDocumentType::NotFoundError, with: :render_not_found | ||
|
|
||
| def choose_type | ||
| @permitted_child_document_types = ConfigurableDocumentType.allowed_child_document_types_of(StandardEdition.find(params[:parent_edition_id])).select { |type| can?(current_user, type) } | ||
|
|
||
| render_not_found if @permitted_child_document_types.empty? | ||
| end | ||
|
|
||
| private | ||
|
|
||
| def edition_class | ||
| StandardEdition | ||
| end | ||
|
|
||
| def new_edition_params | ||
| # Set the configurable document type for new editions based on the value from the query parameter submitted with the 'choose_type' form | ||
| super[:configurable_document_type].blank? ? super.merge(configurable_document_type: params[:configurable_document_type]) : super | ||
| end | ||
|
|
||
| def render_not_found | ||
| render "admin/errors/not_found", status: :not_found | ||
| end | ||
| end | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| module StandardEdition::ChildDocument | ||
| extend ActiveSupport::Concern | ||
|
|
||
| class IncompatibleParentState < ::WhitehallError; end | ||
|
|
||
| included do | ||
| has_one :parent_relationship, | ||
| class_name: "ParentChildRelationship", | ||
| foreign_key: :child_document_id, | ||
| primary_key: :document_id, | ||
| inverse_of: :child_document | ||
|
|
||
| has_one :parent_edition, | ||
|
ChrisBAshton marked this conversation as resolved.
ChrisBAshton marked this conversation as resolved.
|
||
| through: :parent_relationship, | ||
| source: :parent_edition | ||
|
|
||
| before_update :ensure_publishing_is_allowed!, if: :publishing? | ||
| end | ||
|
|
||
| def is_child_document? | ||
| parent_edition.present? | ||
| end | ||
|
|
||
| def parent_allows_publishing? | ||
| return true unless is_child_document? | ||
|
|
||
| parent_edition.document.live_edition&.state == "published" && !parent_edition.pre_publication? | ||
|
ChrisBAshton marked this conversation as resolved.
|
||
| end | ||
|
|
||
| private | ||
|
|
||
| def publishing? | ||
| will_save_change_to_state? && state == "published" | ||
| end | ||
|
|
||
| def ensure_publishing_is_allowed! | ||
| return if parent_allows_publishing? | ||
|
|
||
| raise IncompatibleParentState, "Unable to publish child document" | ||
| end | ||
| end | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,80 @@ | ||
| module StandardEdition::ParentDocument | ||
| extend ActiveSupport::Concern | ||
|
|
||
| class UnableToDelete < ::WhitehallError; end | ||
| class UnableToWithdraw < ::WhitehallError; end | ||
| class UnableToUnpublish < ::WhitehallError; end | ||
|
|
||
| class Trait < Edition::Traits::Trait | ||
| def process_associations_after_save(new_edition) | ||
| ParentChildRelationship | ||
| .where(parent_edition_id: @edition.id) | ||
| .find_each do |relationship| | ||
| ParentChildRelationship.create!( | ||
| parent_edition_id: new_edition.id, | ||
| child_document_id: relationship.child_document_id, | ||
| ) | ||
| end | ||
| end | ||
| end | ||
|
|
||
| included do | ||
| has_many :child_relationships, | ||
| class_name: "ParentChildRelationship", | ||
| foreign_key: :parent_edition_id, | ||
| inverse_of: :parent_edition, | ||
| dependent: :destroy | ||
|
ChrisBAshton marked this conversation as resolved.
|
||
|
|
||
| has_many :child_documents, | ||
| lambda { | ||
| joins(:editions) | ||
| .merge(Edition.all) # applies default scope (i.e. excludes deleted) | ||
|
ChrisBAshton marked this conversation as resolved.
|
||
| .distinct | ||
| }, | ||
| through: :child_relationships, | ||
| source: :child_document | ||
|
|
||
| before_update :ensure_no_new_child_documents!, if: :deleting? | ||
| before_update :ensure_no_children_more_visible_than_parent!, if: :will_save_change_to_state? | ||
|
|
||
| add_trait Trait | ||
| end | ||
|
|
||
| def allows_child_documents? | ||
|
ChrisBAshton marked this conversation as resolved.
|
||
| (type_instance.settings["allowed_child_document_types"] || []).count.positive? | ||
| end | ||
|
|
||
| def is_parent_document? | ||
| child_documents.any? | ||
| end | ||
|
|
||
| def child_editions | ||
| Edition.where(id: child_documents.select(:latest_edition_id)) | ||
| end | ||
|
|
||
| def new_child_documents | ||
| child_documents.where(live_edition_id: nil) | ||
| end | ||
|
|
||
| private | ||
|
|
||
| def deleting? | ||
| will_save_change_to_state? && state == "deleted" | ||
| end | ||
|
|
||
| def ensure_no_new_child_documents! | ||
| if allows_child_documents? && new_child_documents.any? | ||
| raise UnableToDelete, "This document cannot be deleted while it has child documents that have never been published. Delete the draft child documents first." | ||
| end | ||
| end | ||
|
|
||
| def ensure_no_children_more_visible_than_parent! | ||
| if state == "unpublished" && child_editions.any? { |child| child.state.in?(%w[published withdrawn]) } | ||
| raise UnableToUnpublish, "This document cannot be unpublished while it has child documents that are published or withdrawn. Unpublish the child documents first." | ||
| end | ||
|
|
||
| if state == "withdrawn" && child_editions.any? { |child| child.state == "published" } | ||
| raise UnableToWithdraw, "This document cannot be withdrawn while it has child documents that are published. Withdraw the child documents first." | ||
| end | ||
| end | ||
| end | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -59,6 +59,11 @@ def self.where_group(group) | |
| all.filter { |t| t.settings["configurable_document_group"] == group } | ||
| end | ||
|
|
||
| def self.allowed_child_document_types_of(parent_edition) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice one! |
||
| allowed_child_document_types = find(parent_edition.configurable_document_type).settings["allowed_child_document_types"] | ||
| allowed_child_document_types.map { |config| find(config["document_type"]) } | ||
| end | ||
|
|
||
| def self.convertible_from(current_type_key) | ||
| available_types = [] | ||
| if (group = ConfigurableDocumentType.find(current_type_key).settings["configurable_document_group"]) | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
70 changes: 70 additions & 0 deletions
70
app/models/configurable_document_types/topical_event_about_page.json
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,70 @@ | ||
| { | ||
| "key": "topical_event_about_page", | ||
| "title": "Topical event 'About' page", | ||
| "description": "???", | ||
| "forms": { | ||
| "documents": { | ||
| "fields": { | ||
| "body": { | ||
| "title": "Body", | ||
| "description": "The main content of the page", | ||
| "required": true, | ||
| "block": "govspeak", | ||
| "attribute_path": ["block_content", "body"], | ||
| "translatable": true | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| "schema": { | ||
| "attributes": { | ||
| "body": { | ||
| "type": "string" | ||
| } | ||
| }, | ||
| "validations": { | ||
| "presence": { | ||
| "attributes": ["body"] | ||
| }, | ||
| "length": { | ||
| "attributes": ["body"], | ||
| "maximum": 16777215 | ||
| }, | ||
| "safe_html": { | ||
| "attributes": ["body"] | ||
| }, | ||
| "no_footnotes_allowed": { | ||
| "attributes": ["body"] | ||
| }, | ||
| "embedded_contacts_exist": { | ||
| "attributes": ["body"] | ||
| } | ||
| } | ||
| }, | ||
| "presenters": { | ||
| "publishing_api": { | ||
| "details": { | ||
| "body": "govspeak" | ||
| }, | ||
| "links": [ | ||
| "parent" | ||
| ] | ||
| } | ||
| }, | ||
| "settings": { | ||
| "is_child_document": true, | ||
| "publishing_api_schema_name": "topical_event_about_page", | ||
| "publishing_api_document_type": "topical_event_about_page", | ||
| "rendering_app": "frontend", | ||
| "images": { | ||
| "enabled": false | ||
| }, | ||
| "send_change_history": false, | ||
| "file_attachments_enabled": false, | ||
| "organisations": null, | ||
| "backdating_enabled": false, | ||
| "history_mode_enabled": false, | ||
| "taxon_required": false, | ||
| "translations_enabled": true | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Settings for parent and child could fall out of sync across schemas (human error). Could add specific validation to ensure settings match, or use parent's setting, or fall back to parent's when child setting nil.
Do we have any validation here to say that the orgs passed into this setting, for restriction, should also be listed in the org association?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
YAGNI at the moment. We've agreed to let organisations be set individually per parent/child. Out of scope for this PR
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh - I would have thought the implication is the opposite. We said we would allow setting any orgs on child but we won't allow setting access limiting on child. This bit here is more about access so my assumption would be that the org restriction should be based on parent (if access limiting is too)? But we did not touch specifically on org-based access limitations in the session 🤔