Aztec: Implements basic state machine for editor state by astralbodies · Pull Request #6527 · wordpress-mobile/WordPress-iOS

astralbodies · 2017-01-31T21:53:36Z

This adds the Publish button (previously called Post) to the Aztec editor. Instead of using a series of hacky methods to determine the state of the button it extracts the logic into clearly defined units of state that is unit testable. Other variables that affect state of the editor are also in the state context.

This is partially based off of @frosty's PostEditorSaveAction enum and WPPostViewController extension but made to be more abstract.

The main things this solves are:

Publish button title.
Publish button state (enabled/disabled).
What text is shown during the publish process (Publishing...).

What it can/should solve but isn't part of this PR:

Is Post-Post shown?
Is there a secondary publish button and what does it say? (publish now, save draft, etc)
Anything else that's related to the state of the editor that shouldn't turn into a pile of crazy If statements.

Some other TODOs that aren't part of this PR:

Submit for Review is a lot of text to fit on the button label. Maybe just Submit? How are we handling it in cases where the button text is just too long in other languages?
Submit for Review is also not currently supported in the hybrid editor. May need to validate how this all works. Currently the Aztec editor supports it but we're not setting the Bool properly if the user has or doesn't have the permission to publish.
Update the state context when the post is being published to disable the publish button.
Add some more unit tests to make sure all state switches are accounted for.

How to Test

Make sure the publish button shows the right text and is enabled in these scenarios:

New post, no content. (Publish, disabled)
New post, with content. (Publish, enabled)
New post, set to draft. (Save)
New post, future dated. (Schedule)
New post, past dated. (Publish)
Existing published post. (Update)
Existing scheduled post. (Update)
Existing draft post. (Save)

There are a bunch of other scenarios but you get the point.

Needs review: @frosty, @jleandroperez

# Conflicts: # WordPress/Classes/ViewRelated/Post/AztecPostViewController.swift

jleandroperez

Beautiful code!! Adding just a few comments. About to begin testing on the actual device. Zero blockers so far!

jleandroperez · 2017-01-31T22:22:23Z

WordPress/Classes/ViewRelated/Post/AztecPostViewController.swift

+            originalPostStatus = PostStatus(rawValue: postStatus) ?? .draft
+        } else {
+            originalPostStatus = .draft
+        }


Nipticky // feel free to disregard!!

Potential candidate for a helper method:

enum PostStatus { static func fromPost(post: Post) -> PostStatus { //... } }

Could be a good idea – defaulting to .draft. Could also be used in observeValue(forKey:...)

jleandroperez · 2017-01-31T22:24:25Z

WordPress/Classes/ViewRelated/Post/AztecPostViewController.swift

+
        super.init(nibName: nil, bundle: nil)
+
+        addObservers(toPost: post)


I think it's safe to remove this addObservers, since it's already being done in the didSet.

Unless i'm missing something!

@jleandroperez didSet isn't called when an init method sets the properties before super is called. Once super is called then didSet is called every future time the property is set. Stabby!!

jleandroperez · 2017-01-31T22:34:21Z

WordPress/Classes/ViewRelated/Post/PostEditorState.swift

+    fileprivate var isBeingPublished = false {
+        didSet {
+            if isBeingPublished != oldValue {
+                publishActionAllowed = hasContent && !isBeingPublished


Still thinking if this snippet deserves a method its own method, or not (refreshActionAllowedState). It's just a line... perhaps i'm overthinking this one!

jleandroperez · 2017-01-31T22:35:40Z

WordPress/Classes/ViewRelated/Post/PostEditorState.swift

+    }
+
+    var publishButtonText: String {
+        return editorState.action().publishActionLabel


action.publishActionLabel

jleandroperez · 2017-01-31T22:35:55Z

WordPress/Classes/ViewRelated/Post/PostEditorState.swift

+    }
+
+    var publishVerbText: String {
+        return editorState.action().publishingActionLabel


action.publishingActionLabel (nipticky, sorry!)

jleandroperez · 2017-01-31T22:36:06Z

WordPress/Classes/ViewRelated/Post/PostEditorState.swift

+    }
+
+    var isPostPostShown: Bool {
+        return editorState.action().isPostPostShown


action.isPostPostShown (sorry, nipticky++)

Agreed - I'll turn action() into a protocol property.

# Conflicts: # WordPress/Classes/ViewRelated/Post/AztecPostViewController.swift

jleandroperez · 2017-02-01T09:09:15Z

Note 1:

Drafts are displaying the Publish Action (vs Update in the Hybrid editor):

In a testing blog, create a Draft
Enable Aztec
Edit the Post. The top right action will read Publish

Is this expected?

Update: Possibly related to #6534

…atus-state-machine

jleandroperez · 2017-02-01T10:58:59Z

Note 2:

We've got an overlap between navigationBar Action Button, and the Aztec Title. We should either cap the Blog Title's Width, or indicate somehow else that we're in Aztec. (I'd vote for both perhaps!)

jleandroperez · 2017-02-01T11:09:39Z

Note 3:

The post's setter observer is not refreshing the PostEditorStateContext. I suspect we should be re-instantiating the PostEditorStateContext's instance, whenever the Post changes.

IE: From Revision to Original.

jleandroperez · 2017-02-01T11:12:31Z

Note 4:

Post's Status Context didChangeAction delegate method (which is in charge of setting the Publish Button's Title) is not getting called with the initial value.

We could either mimic KVO's "Post Initial Value" flag, or just add that to the refreshInterface helper?

frosty · 2017-02-01T13:53:20Z

A few notes from putting this through its paces:

When writing a new post, the Publish button is only enabled when text is added to the body of the post, not the title. On the hybrid editor, typing characters into the title field enables the button. We should probably do the same in Aztec? (you could save a post with only a title)
If I set a new post to Scheduled (setting a publish date), the button changes to Schedule (as expected). If I change the post back to Published (by tapping Publish Immediately), the button title stays as Schedule. In fact, even if I change the post status to Draft the title is still Schedule. Not the case with the hybrid editor.
I'm seeing the same as @jleandroperez – existing posts are all showing Publish for me. I tested with existing drafts, scheduled posts, and published posts. The button says Publish for all of them.
The existing button says "Post" not "Publish". Are we changing this?

Taking a look through the code now... :)

frosty

This is looking great, @astralbodies! I've left some code comments, and it seems like @jleandroperez has everything else covered :)

frosty · 2017-02-01T14:35:22Z

WordPress/WordPressTest/PostEditorStateTests.swift

+    }
+
+    func testContextChangedNewPostToDraft() {
+        context = PostEditorStateContext(originalPostStatus: .draft, userCanPublish: true, delegate: self)


Isn't a new post created with publish status, not draft?

(although note that if you change this, we already have a publish -> draft test below.

Just tested with the existing editor. Posts begin in the publish status. If you change it to draft, the button shows Save.

I fixed this with a check-in - new posts will pass in a nil originalPostStatus now since it's not on the server. I was trying to match logic in Calypso which makes every post a draft when creating it which is something we should eventually emulate. 😄

I definitely agree we should start every post off as a draft!

frosty · 2017-02-01T14:38:21Z

WordPress/WordPressTest/PostEditorStateTests.swift

+    func testContextDraftPost() {
+        context = PostEditorStateContext(originalPostStatus: .draft, userCanPublish: true, delegate: self)
+
+        XCTAssertEqual(PostEditorAction.publish, context.action, "should return Publish if the post is a draft")


The hybrid editor shows Update if the post is a draft.

frosty · 2017-02-01T14:41:14Z

WordPress/WordPressTest/PostEditorStateTests.swift

+        XCTAssertEqual(PostEditorAction.submitForReview, context.action, "should return 'Submit for Review' if the post is a draft and user can't publish")
+    }
+
+


Should we add another test ensuring that a post originally in the scheduled status, if we don't change the date at all, shows an update action?

Added unit tests to cover these scenarios.

frosty · 2017-02-01T14:42:49Z

WordPress/WordPressTest/PostEditorStateTests.swift

+
+    // Missing test: should return false if not dirty and has no content
+
+    func testPublishDisabledNoContent() {


This is very similar to the first test (testContextNoContentPublishButtonDisabled). Perhaps we should name them more differently or more consistently, and perhaps group them together.

frosty · 2017-02-01T15:04:35Z

WordPress/Classes/ViewRelated/Post/PostEditorState.swift

+        }
+    }
+
+    var isPostPostShown: Bool {


Looks like this isn't used yet and I might be misunderstanding its use, but with the hybrid editor we also show post-post for scheduled posts.

It's not currently used yet - will be in another PR.

frosty · 2017-02-01T15:08:13Z

WordPress/Classes/ViewRelated/Post/PostEditorState.swift

+
+    fileprivate var hasContent = false {
+        didSet {
+            publishActionAllowed = hasContent && !isBeingPublished


I know it's only one line, but we could extract this to a private updatePublishActionAllowed func and call it from both didSets?

Yup I'll make that change. 👍

frosty · 2017-02-01T15:20:40Z

WordPress/Classes/ViewRelated/Post/PostEditorState.swift

+    }
+
+    func updated(postStatus: PostStatus, context: PostEditorStateContext) -> PostEditorActionState {
+        return self


Presumably this is why I can't seem to get the button out of Schedule state once I've entered it?

frosty · 2017-02-01T15:32:21Z

WordPress/Classes/ViewRelated/Post/PostEditorState.swift

+fileprivate extension PostEditorActionState {
+    func isFutureDated(_ date: Date) -> Bool {
+        let oneMinute: TimeInterval = 60.0
+        let dateOneMinuteFromNow = Date(timeInterval: oneMinute, since: Date())


I would instead use Calendar.current.date(byAdding: .minute, value: 1, to: date)

Or you could potentially use Calendar.current.compare(Date(), to: date, toGranularity: .minute) to compare them instead.

frosty · 2017-02-01T15:33:39Z

WordPress/Classes/ViewRelated/Post/AztecPostViewController.swift

+    }
+
+    internal func context(_ context: PostEditorStateContext, didChangeAction: PostEditorAction) {
+        publishButton.title = context.publishButtonText


In both of these delegate methods, is there a reason you're going via the context instead of using the passed in action / bool?

…ded.

astralbodies · 2017-02-01T16:08:56Z

Responding to @frosty's main thread comments:

A few notes from putting this through its paces:

When writing a new post, the Publish button is only enabled when text is added to the body of the post, not the title. On the hybrid editor, typing characters into the title field enables the button. We should probably do the same in Aztec? (you could save a post with only a title)

Done. Had to add a target/action for the editing changed event on the textview to watch for this. The postTitle property on the post isn't updated until editing is done.

If I set a new post to Scheduled (setting a publish date), the button changes to Schedule (as expected). If I change the post back to Published (by tapping Publish Immediately), the button title stays as Schedule. In fact, even if I change the post status to Draft the title is still Schedule. Not the case with the hybrid editor.

Fixed.

I'm seeing the same as @jleandroperez – existing posts are all showing Publish for me. I tested with existing drafts, scheduled posts, and published posts. The button says Publish for all of them.

Fixed.

The existing button says "Post" not "Publish". Are we changing this?

I'm forcing us to match what Calypso says. I think @mattmiklic agreed when I asked about it. 😄

astralbodies · 2017-02-01T16:13:37Z

@jleandroperez Here are responses to your notes:

Note 1:
Drafts are displaying the Publish Action (vs Update in the Hybrid editor):

Fixed.

Note 2:
We've got an overlap between navigationBar Action Button, and the Aztec Title. We should either cap the Blog Title's Width, or indicate somehow else that we're in Aztec. (I'd vote for both perhaps!)

We talked about this and removing the navbar title makes the most sense. It's getting in the way and we'll mark the editor as being Aztec is some other way like slightly changing the title bar color or something. Will address in a different PR.

Note 3:
The post's setter observer is not refreshing the PostEditorStateContext. I suspect we should be re-instantiating the PostEditorStateContext's instance, whenever the Post changes.

For now I moved post revision creation to happen before the UI is set up. I think it makes sense we do the revision operation first.

Note 4:
Post's Status Context didChangeAction delegate method (which is in charge of setting the Publish Button's Title) is not getting called with the initial value.

I think letting the context emit the current state is a good idea. (There's a similar feature in RxSwift ... just sayin...) For now I'll leave it as there are a few things I want to add to the context (like the secondary publish button and post-post functionality) and I'll add this in.

astralbodies · 2017-02-01T16:34:15Z

@jleandroperez @frosty I think this is ready for another pass.

mattmiklic · 2017-02-01T16:53:34Z

I'm forcing us to match what Calypso says. I think @mattmiklic agreed when I asked about it. 😄

💯. Post in the iOS editor is an outlier; both Calypso and wp-android use Publish.

frosty · 2017-02-02T17:53:02Z

@astralbodies Changes look good! 👍 for all the comments you've added. Fixes the issues I had, and the tests all pass.

from me, after the conflicts are fixed!

# Conflicts: # WordPress/Classes/ViewRelated/Post/AztecPostViewController.swift

jleandroperez · 2017-02-02T21:13:36Z

Beautiful code @astralbodies, all looking good!!

!!!

astralbodies · 2017-02-02T21:18:37Z

Thank you both so much for the time to review!

astralbodies added 16 commits January 16, 2017 11:50

Initial design for state machine for post status

a2b52c7

Swift 3 things

be5d8d7

Tweaks

820e3db

Certificate updates

3ad99ac

Added a delegate

733ecf8

Merge branch 'develop' into issue/6422-post-status-state-machine

9c8a45f

Wiring in state machine

886202a

Wiring up more of the state machine to the Aztec VC

a83ea34

Added basic unit tests and more integration. We're getting there!

9fbdf9e

Straightened out actions/states to align properly with editor.

3653132

Clarifying original post status and unit tests.

ac51dda

Shored up some internals, added unit tests (most fail - not implemented)

07bb9e9

Finished implementing unit tests from Calypso - skipped a few

b98cf30

Watching for changes in posts

19d596b

Merge branch 'develop' into issue/6422-post-status-state-machine

3768b85

# Conflicts: # WordPress/Classes/ViewRelated/Post/AztecPostViewController.swift

Wired up observation for content & post status

18f7f96

astralbodies added the Aztec label Jan 31, 2017

astralbodies added this to the Aztec v1 milestone Jan 31, 2017

astralbodies assigned frosty, astralbodies and jleandroperez Jan 31, 2017

astralbodies requested review from frosty and jleandroperez January 31, 2017 21:53

astralbodies changed the title ~~Implements basic state machine for editor state~~ Aztec: Implements basic state machine for editor state Jan 31, 2017

Added code comments

199ce1a

jleandroperez reviewed Jan 31, 2017

View reviewed changes

astralbodies added 4 commits January 31, 2017 16:41

Don't need dynamic because KVO is on the object, not the property.

e032418

Properly handle state change for button enabled.

d3a4741

Turn action() into a protocol property.

40add3a

Merge branch 'develop' into issue/6422-post-status-state-machine

9b474af

# Conflicts: # WordPress/Classes/ViewRelated/Post/AztecPostViewController.swift

Merge remote-tracking branch 'origin/develop' into issue/6422-post-st…

68f9ef9

…atus-state-machine

Makes original post status an optional when no original post exists.

e2c75bd

frosty reviewed Feb 1, 2017

View reviewed changes

astralbodies added 5 commits February 1, 2017 09:34

Looks at title for a post having content. Placeholders for isDirty ad…

4834b67

…ded.

Reduce duplication of code in publish action state.

2a1ee02

Handles switching future dated post back to current & button to Publish.

3ec2a78

Improved the date/time comparison logic.

c194835

Unit test scheduled post state changes.

aedf26f

Added a bunch of comments for clarity.

fa6df5c

Merge branch 'develop' into issue/6422-post-status-state-machine

49db40d

# Conflicts: # WordPress/Classes/ViewRelated/Post/AztecPostViewController.swift

astralbodies merged commit cde4de0 into develop Feb 2, 2017

astralbodies deleted the issue/6422-post-status-state-machine branch February 2, 2017 21:18

astralbodies mentioned this pull request Feb 9, 2017

Aztec: Add Post / Save / Update button to the top toolbar (and its logic) #5939

Closed


		super.init(nibName: nil, bundle: nil)

		addObservers(toPost: post)

		XCTAssertEqual(PostEditorAction.submitForReview, context.action, "should return 'Submit for Review' if the post is a draft and user can't publish")
		}


		// Missing test: should return false if not dirty and has no content

		func testPublishDisabledNoContent() {

Conversation

astralbodies commented Jan 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

How to Test

Uh oh!

jleandroperez left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jleandroperez Jan 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jleandroperez commented Feb 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Note 1:

Uh oh!

jleandroperez commented Feb 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Note 2:

Uh oh!

jleandroperez commented Feb 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Note 3:

Uh oh!

jleandroperez commented Feb 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Note 4:

Uh oh!

frosty commented Feb 1, 2017

Uh oh!

frosty left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

astralbodies commented Jan 31, 2017 •

edited

Loading

jleandroperez Jan 31, 2017 •

edited

Loading

jleandroperez commented Feb 1, 2017 •

edited

Loading

jleandroperez commented Feb 1, 2017 •

edited

Loading

jleandroperez commented Feb 1, 2017 •

edited

Loading

jleandroperez commented Feb 1, 2017 •

edited

Loading

mattmiklic commented Feb 1, 2017 •

edited

Loading