Skip to content
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

Remove Embargo & Expiry feature from codebase. Add support for external module #389

Open
wants to merge 3 commits into
base: 5
Choose a base branch
from

Conversation

chrispenny
Copy link

@chrispenny chrispenny commented Mar 15, 2019

The Gist

Remove the Embargo & Expiry feature from code base, and add support for external Embargo & Expiry module.

The Embargo & Expiry module already supports the Fluent module.

WorkflowEmbargoExpiryExtension.php

The DB fields between Advanced Workflow and Embargo & Expiry match, so they should not require any "migration", so long as devs apply the EmbargoExpiryExtension to the same model that previously had WorkflowEmbargoExpiryExtension.

Migration dev task

The DB fields above are fine, but if the site already has some existing jobs from this module, then those will need to be migrated to jobs from the Embargo & Expiry module.

EmbargoExpiryMigrationTask has been provided for this.

Below are the tests I went though. I started with Workflow's Embargo feature, and I then switched to Embargo & Expiry, ran a dev/build plus the above dev task. Below indicates the "state" that the page was in when I switched modules. Each page remained in the desired state after the migration.

  • Page with no workflow saved with E&E: Expected Jobs to be created
  • Page with workflow saved (but not submitted) with E&E: Expected no Jobs but saved "Desired" dates
  • Page with workflow submitted (but not approved) with E&E: Expected no Jobs but saved "Desired" dates
  • Page with workflow submitted and approved with E&E: Expected Jobs to be created

Before:
Screen Shot 2022-07-19 at 11 13 54
Screen Shot 2022-07-19 at 11 14 34

After:
Screen Shot 2022-07-19 at 11 28 14
Screen Shot 2022-07-19 at 11 28 30

How we're doing it

Embargo & Expiry provides an extension point (preventEmbargoExpiryQueueJobs ) which can be used to interrupt to queueing of Jobs when a record is saved.

If a DataObject has a Workflow applied, then we want to interrupt Job creation so that we can first go through the approval process. Once the process is completed, and the "Un/Publish" action is triggered, then we can queue the Jobs.

PublishItemWorkflowAction.php & UnpublishItemWorkflowAction.php

When either of these actions are submitted, we check for the existence of the required Extensions as well as any Desired embargo/expiry dates. If some are found, then we queue up the appropriate Jobs, rather than publish/unpublish at the time of the actions trigger.

Delay action fields

When publishing, if the Action has a PublishDelay, then we will queue up that PublishDelay as an embargo date. This will override any DesiredPublishDate that might have been set.

Similar process for unpublishing and the UnpublishDelay.

WorkflowPublishTargetJob.php

Removed. No longer required as this process is taken care of by Embargo & Expiry module.

WorkflowEmbargoExpiryTest.php

Updated to only cover the areas that this module is now interested in. Which is to say, that the module now only needs to care that Jobs are correctly created (or not created) when approvals are made.

@chrispenny chrispenny changed the title WIP Remove Embargo & Expiry feature from codebase. Add support for external module Remove Embargo & Expiry feature from codebase. Add support for external module Mar 15, 2019
Copy link
Contributor

@ScopeyNZ ScopeyNZ left a comment

Choose a reason for hiding this comment

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

The PHP support question is a good one. I personally feel like we should be able to release a major version of this module after June and set a minimum version of 7.2. That's just my opinion though 🙂

I haven't checked this out locally for testing just given it a quick once over on the code.

.gitignore Outdated Show resolved Hide resolved
composer.json Outdated Show resolved Hide resolved
@chrispenny
Copy link
Author

Thanks, @ScopeyNZ!

Sorry that took me forever. This is my first hackday since my hiatus.

@chrispenny
Copy link
Author

chrispenny commented Apr 27, 2022

Is it worth me picking this up again?

The original blocker isn't an issue any more, but I would need to see where I've taken Embargo & Expiry in the last few years to make sure things are still compatible.

@chrispenny chrispenny force-pushed the feature/support-external-embargo-module branch from 43e5dfd to c8767a6 Compare July 18, 2022 22:10
@chrispenny chrispenny changed the base branch from master to 5 July 18, 2022 22:12
@chrispenny chrispenny force-pushed the feature/support-external-embargo-module branch from c8767a6 to db08b33 Compare July 18, 2022 22:19
@chrispenny chrispenny force-pushed the feature/support-external-embargo-module branch from 91bad6a to e0d7075 Compare July 19, 2022 01:06
@chrispenny
Copy link
Author

Hi team,

This pull request has been updated to match the current state of both modules. I wasn't sure if this should target 5 or master. master looks to be very far behind.

I would leave it up to you to decide (of course), but I'm guessing this would need to be a new MAJOR. The DB fields themselves map fine, but existing Jobs needs to be migrated. I've provided some documentation and a dev task for folks to do that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants