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

RFC:21 Revision scheduled publishing #4050

Closed
wants to merge 12 commits into
base: master
from

Conversation

5 participants
@hakjoon
Copy link
Contributor

hakjoon commented Nov 21, 2017

This PR adds functionality for Wagtail RFC 21 allowing for scheduling revisions without unpublishing currently live pages.

@hakjoon hakjoon force-pushed the themotleyfool:revision-scheduled-publishing branch from a886ef4 to c8ef88b Nov 27, 2017

@hakjoon hakjoon force-pushed the themotleyfool:revision-scheduled-publishing branch from c8ef88b to 7c82dbc Nov 28, 2017

@gasman gasman added this to the 2.0 milestone Dec 6, 2017

@gasman gasman self-requested a review Dec 6, 2017

@hakjoon

This comment has been minimized.

Copy link
Contributor

hakjoon commented Dec 7, 2017

The current implementation does not execute on the [ Live + Scheduled + Draft ] status suggested in the RFC. It does create a [ Live + Scheduled ] status but I couldn't figure out a way to see if the scheduled revision was the latest revision without some changes to the Page model which I was hesitant to do. The behavior does work like the current [Scheduled] status which does not indicate if a draft exists beyond the scheduled revision.

There is an indicator in the revisions index for the scheduled revision so it is fairly easy to see where it stands in the revisions stack.

@hakjoon hakjoon changed the title [WIP] RFC:21 Revision scheduled publishing RFC:21 Revision scheduled publishing Dec 7, 2017

@gasman gasman added this to Planned for 2.0 in Roadmap Dec 13, 2017

if not user_perms.for_page(page).can_unpublish():
raise PermissionDenied

revision = get_object_or_404(page.revisions, id=revision_id)

This comment has been minimized.

@lb-

lb- Dec 18, 2017

Contributor

Should check here if this revision is actually a future scheduled revision.
If not - it should redirect the user back to the revisions page, maybe with a warning that there is no revision to unschedule.

@lb-

This comment has been minimized.

Copy link
Contributor

lb- commented Dec 18, 2017

@hakjoon I took a look at this tonight, checked out the PR locally and also read through the code changes, below are some comments/thoughts/screenshots.

This is an awesome feature! Awesome work on this.

1. revisions_unschedule view/method should check for invalid URL

This view should check if there is an actual unschedule action that can occur, otherwise it might make for a confusing user experience in edge cases.

Let's say you happen to open the 'unschedule revision' link (or copy to clipboard), but the revision has already been unpublished when you actually open the URL. There is no indicator that you cannot do this action as it has already been done.

2. Live + Scheduled + Draft Indicator

After using this feature, I do think this is needed, but I am unsure as to the model changes needed.

3. More specific success message

When a page is live and you are scheduling a new page for the future, the success message says:

Page 'About' has been scheduled for publishing.

Maybe this should be more specific to this being a live page with a future schedule. Rough idea below.

Page 'About' is live and this revision has been scheduled for publishing.

Screenshots

No comments, just showing it all works as expected.

wagtail - pr4050 - new sheduled page
wagtail - pr4050 - scheduled - explorer menu
wagtail - pr4050 - draft - revisions list

@hakjoon

This comment has been minimized.

Copy link
Contributor

hakjoon commented Dec 18, 2017

@lb- Thank you for the feedback.

revisions_unschedule view/method should check for invalid URL

I think this makes sense. I originally thought of adding this but based the view on the unpublish view which did not have a similar check, so ended up leaving it out. My thought was that one would not get to it without having a link from a scheduled revision, but your point about bookmarks, url completion is very valid. Feels like it should be an easy enough change.

More specific success message

I believe this should be easy enough to accommodate.

Live + Scheduled + Draft Indicator

I wanted to see what the general feeling was without this since we don't currently have it for scheduled pages, but I should probably go into more detail as to what I think the required changes are.

From my digging I think we would have to expose something akin to a get_scheduled_revision method that would return the scheduled revision. At that point we could check if the scheduled_revision == latest_revision. There already is an approved_schedule property that does an exists() check for a scheduled revision. I wasn't sure if we would want to modify the semantics of that property to return the actual revision or add a second accessor.

A second accessor would be simple but it does start polluting the API for the page model a bit and changing the existing property does change the public API so I was hesitant to do it without some buy in. The change would allow for a [scheduled + draft] status for non-live pages also.

@lb-

This comment has been minimized.

Copy link
Contributor

lb- commented Dec 19, 2017

One other thing that is needed is some additional docs relating to this feature:

Also see #2841 for general older feedback on the docs surrounding scheduled publishing.

I can see that the docs for scheduled publishing are pretty sparse but it would be good to cover a few of these gaps now that this feature will be slightly more complex.

@tomdyson

This comment has been minimized.

Copy link
Contributor

tomdyson commented Dec 20, 2017

This looks great @hakjoon! And thanks for the review, @lb-.

Paging @Gagaro - could your impressive https://github.com/Gagaro/wagtail-calendar be aware of scheduled revisions?

@Gagaro

This comment has been minimized.

Copy link
Contributor

Gagaro commented Dec 20, 2017

I'll definitely try to implement this. But I'm still missing time. I wanted to add more tests and documentation before releasing. I probably won't be able to do it before another few weeks.

@tomdyson

This comment has been minimized.

Copy link
Contributor

tomdyson commented Jan 3, 2018

@hakjoon Happy New Year! Are you able to make a start on the docs for your changes, along the lines of @lb-'s suggestions?

@hakjoon

This comment has been minimized.

Copy link
Contributor

hakjoon commented Jan 4, 2018

@tomdyson I'll probably have time to start on docs next week.

@tomdyson

This comment has been minimized.

Copy link
Contributor

tomdyson commented Jan 10, 2018

@hakjoon have you had a chance to look at docs? We're really keen to get this in 2.0 if possible. Thanks!

@hakjoon

This comment has been minimized.

Copy link
Contributor

hakjoon commented Jan 10, 2018

Not yet I’ve been short on time this week.

@hakjoon

This comment has been minimized.

Copy link
Contributor

hakjoon commented Jan 11, 2018

Pushed an initial stab at documentation. Fair warning I'm pretty bad at writing docs.

I also added a more specific message when scheduling a revision for a page that is already live per @lb- suggestion.

Regarding revisions_unschedule view/method should check for invalid URL currently if you try to unschedule a revision that is not scheduled nothing really happens since all it does is clear approved_go_live which in this case would not be set anyway so not sure if it is worth trying to handle the possibility of a bookmark, clipboard mishap.

@tomdyson
Copy link
Contributor

tomdyson left a comment

Thanks @hakjoon!

~~~~~~~~~~~~~~~
================
Edit Page tabs
================

A common feature of the *Edit* pages for all page types is the two tabs at the top of the screen. The first, Content, is where you build the content of the page itself.

This comment has been minimized.

@tomdyson

tomdyson Jan 11, 2018

Contributor

two tabs -> three tabs

~~~~~~~~~~~~~~~
================
Edit Page tabs
================

A common feature of the *Edit* pages for all page types is the two tabs at the top of the screen. The first, Content, is where you build the content of the page itself.

This comment has been minimized.

@tomdyson

tomdyson Jan 11, 2018

Contributor

Content -> Content

The Settings Tab
~~~~~~~~~~~~~~~~

The *Settings* has two fields by default.

This comment has been minimized.

@tomdyson

tomdyson Jan 11, 2018

Contributor

Settings -> Settings tab


The *Settings* has two fields by default.

* **Go Live date/time:** Sets the time in which the changes should go live when published. If you publish a page that has this field set to a time in the future it will be scheduled until the time comes.

This comment has been minimized.

@tomdyson

tomdyson Jan 11, 2018

Contributor

time in which -> time at which
I suggest removing the second sentence "If you publish a page that has this field set to a time in the future it will be scheduled until the time comes."


* **Go Live date/time:** Sets the time in which the changes should go live when published. If you publish a page that has this field set to a time in the future it will be scheduled until the time comes.
See :ref:`scheduled_publishing` for more details.
* **Expiry date/time:** Sets the time in which this page should be unpublished.

This comment has been minimized.

@tomdyson

tomdyson Jan 11, 2018

Contributor

time in which -> time at which

scheduled time.
In order for pages to be published at the scheduled time you should set up the :ref:`publish_scheduled_pages` management command.

The basic workflow is as follows.

This comment has been minimized.

@tomdyson

tomdyson Jan 11, 2018

Contributor

as follows. -> as follows:


The basic workflow is as follows.

* Scheduling a revision for a page that is not currently live schedules that page to go live when the scheduled time comes.

This comment has been minimized.

@tomdyson

tomdyson Jan 11, 2018

Contributor

schedules that page to go live -> means that page will go live

The basic workflow is as follows.

* Scheduling a revision for a page that is not currently live schedules that page to go live when the scheduled time comes.
* Scheduling a revision for a page that is already live schedules that revision to update the page when the time comes.

This comment has been minimized.

@tomdyson

tomdyson Jan 11, 2018

Contributor

schedules that revision to update the page -> means that revision will be published


* Scheduling a revision for a page that is not currently live schedules that page to go live when the scheduled time comes.
* Scheduling a revision for a page that is already live schedules that revision to update the page when the time comes.
* If page has a scheduled revision and you set another revision to publish immediately the scheduled revision will be unscheduled.

This comment has been minimized.

@tomdyson

tomdyson Jan 11, 2018

Contributor

immediately the scheduled -> immediately, the scheduled

* Scheduling a revision for a page that is already live schedules that revision to update the page when the time comes.
* If page has a scheduled revision and you set another revision to publish immediately the scheduled revision will be unscheduled.

The *Revisions* view for a given page will show which revision is scheduled and for when it is scheduled. A scheduled revision in teh list will also provide an *Unschedule* button to cancel it.

This comment has been minimized.

@tomdyson

tomdyson Jan 11, 2018

Contributor

for when it is scheduled -> when it is scheduled for
teh list -> the list :)

@hakjoon

This comment has been minimized.

Copy link
Contributor

hakjoon commented Jan 11, 2018

@tomdyson thanks for the edits. Changes pushed.

@@ -102,17 +102,17 @@ You can apply custom behavior to this process by overriding ``Page`` class metho
Scheduled Publishing
~~~~~~~~~~~~~~~~~~~~

Page publishing can be scheduled through the *Go live date/time* feature in the *Settings* tab of the *Edit* page. This allows you to set set up initial page publishing or a page update in advance and it will only happen at the
Page publishing can be scheduled through the *Go live date/time* feature in the *Settings* tab of the *Edit* page. This allows you to set set up initial page publishing or a page update in advance.
scheduled time.

This comment has been minimized.

@tomdyson

tomdyson Jan 11, 2018

Contributor

'scheduled time.' is still hanging

This comment has been minimized.

@hakjoon

hakjoon Jan 11, 2018

Contributor

argh, fix pushed.

@tomdyson
Copy link
Contributor

tomdyson left a comment

Thanks, @hakjoon, this looks good apart from the extra line.

@hakjoon hakjoon force-pushed the themotleyfool:revision-scheduled-publishing branch from ed91ccd to 60889a7 Jan 11, 2018

gasman added a commit that referenced this pull request Jan 16, 2018

@gasman

This comment has been minimized.

Copy link
Collaborator

gasman commented Jan 16, 2018

Merged in 8c0b9b0 + parents - thanks @hakjoon!

@gasman gasman closed this Jan 16, 2018

@gasman gasman moved this from Planned for 2.0 to In 2.0 in Roadmap Jan 16, 2018

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