-
Notifications
You must be signed in to change notification settings - Fork 6
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
COR-710: Publish System Rebuild #487
Conversation
197fac5
to
5f64af6
Compare
= form.button 'Save as Draft', value: 'draft', name: 'content_item[state]', class: 'mdl-button mdl-js-button mdl-button--cb mdl-js-ripple-effect', type: 'submit' | ||
- else | ||
= form.submit 'Submit', class: 'mdl-button mdl-js-button mdl-button--raised mdl-button--success mdl-js-ripple-effect' | ||
= form.submit 'Save Now', class: 'mdl-button mdl-js-button mdl-button--raised mdl-button--success mdl-js-ripple-effect' |
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.
I believe we'll still need to reflect the state in the button's text, per Justin's previous AC
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.
I talked with him about this, since the buttons wouldn't actually move the state along he preferred if it just said "Save Now" until we figure out how best to implement the triggered state changes
@@ -4,20 +4,7 @@ var datepicker_init = function() { | |||
if (datepicker.length) { | |||
datepicker.datetimepicker({ | |||
dateFormat: 'dd/mm/yy', | |||
onSelect: function (date) { |
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.
We still need to reflect the latest state as the user changes publish/expiration dates, as per Justin's previous AC
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.
I talked with him about this, since the buttons wouldn't actually move the state along he preferred if it just said "Save Now" until we figure out how best to implement the triggered state changes
@@ -0,0 +1,27 @@ | |||
class PublishStateService < ApplicationController |
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.
Let's not inherit from ApplicationController
here - this brings in a lot of stuff. We can follow our existing service data object pattern - inherit from ApplicationService
and specify your Service's attributes
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.
Whoops - that's a typo, good catch
@@ -0,0 +1,27 @@ | |||
class PublishStateService < ApplicationController | |||
def perform(sorted_field_items, content_item) |
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.
I'm guessing this returns the publish status of a ContentItem
- if so, we'll want to make the method name reflect that
|
||
private | ||
|
||
def active_state |
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.
Since our terminology is 'published', should this be active_state
?
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.
Our terminology is only published for this specific ContentType
, this method is only trying to figure out which, of the potentially multiple states, is presently active
app/models/content_item.rb
Outdated
def schedule_publish | ||
timestamp = field_items.find { |field_item| field_item.field.name == "Publish Date" }.data["timestamp"] | ||
PublishContentItemJob.set(wait_until: DateTime.parse(timestamp)).perform_later(self) | ||
sorted_field_items = field_items.select { |fi| fi.field.field_type_instance.is_a?(DateTimeFieldType) && !fi.field.metadata["state"].nil? }.sort_by{ |a| a.data["timestamp"] }.reverse |
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.
I have some thoughts about this system (which is pretty neat!), but we may end up rebuilding some of this anyway - so we can discuss 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.
Let's move this logic to the Service layer - consumers of this abstraction ideally shouldn't have to know how to groom the data being passed in. This issue of 'leaky abstractions' is something that I'm trying to correct across the application - it's hard to stay ontop of, and it's something we've all been guilty of. Thanks!
@toastercup this is ready for re-review |
app/models/content_item.rb
Outdated
@@ -115,4 +96,12 @@ def update_tag_lists | |||
ContentItemService.update_tags(self, tags) | |||
end | |||
end | |||
|
|||
def method_missing(*args) |
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.
Since method_missing
can't be self-descriptive, would you mind documenting what this will give us?
app/models/content_item.rb
Outdated
def schedule_publish | ||
timestamp = field_items.find { |field_item| field_item.field.name == "Publish Date" }.data["timestamp"] | ||
PublishContentItemJob.set(wait_until: DateTime.parse(timestamp)).perform_later(self) | ||
sorted_field_items = field_items.select { |fi| fi.field.field_type_instance.is_a?(DateTimeFieldType) && !fi.field.metadata["state"].nil? }.sort_by{ |a| a.data["timestamp"] }.reverse |
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.
Let's move this logic to the Service layer - consumers of this abstraction ideally shouldn't have to know how to groom the data being passed in. This issue of 'leaky abstractions' is something that I'm trying to correct across the application - it's hard to stay ontop of, and it's something we've all been guilty of. Thanks!
@@ -0,0 +1,21 @@ | |||
class PublishStateService < ApplicationService | |||
def content_item_state(sorted_field_items, content_item) |
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.
As PublishStateService
will always concern itself with ContentItems
and FieldItems
, let's make these two parameters Virtus attributes that methods across the service can refer to
@toastercup this is ready for re-review |
bcc7e57
to
7900600
Compare
Purpose:
This PR is for a rebuild / reimagining of the Cortex Publish System for created content items. The way this new system works is by using flexibly defined states from the decorator, rather than hard coded state machine states, then determining the current state of a content_item using the relevant date time fields. This is replacing our ActiveJob enqueued approach to handling scheduling and publishing.
Additional inclusions in this PR would be the new names for Employer Blog Categories in the Category tree, which was from a minor task by @justin3thompson, AND adds back the expiration date, since this new system makes that concern extremely simple.
JIRA:
https://cb-content-enablement.atlassian.net/browse/COR-710
Steps to Take On Prod
Changes:
Changes to setup
Architectural changes
Migrations
Library changes
Side effects
Screenshots
Before
N/A
After
N/A
QA Links:
http://web.cortex-7.development.c66.me/
How to Verify These Changes
Specific pages to visit
Steps to take
Responsive considerations
Relevant PRs/Dependencies:
N/A
Additional Information
N/A