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

LabeledEnum helper property to replace Docflow #150

Closed
jace opened this issue Oct 30, 2017 · 12 comments
Closed

LabeledEnum helper property to replace Docflow #150

jace opened this issue Oct 30, 2017 · 12 comments

Comments

@jace
Copy link
Member

jace commented Oct 30, 2017

LabeledEnum is now consistently used in HasGeek apps for status flag values. With the support for grouped values in #148, it now replaces the workflow state definition feature in Docflow.

LabeledEnum version:

class POSTSTATUS(LabeledEnum):
    DRAFT =        (0,  __("Draft"))         # Being written
    PENDING =      (1,  __("Pending"))       # Pending email verification
    CONFIRMED =    (2,  __("Confirmed"))     # Post is now live on site
    REVIEWED =     (3,  __("Reviewed"))      # Reviewed and cleared for push channels

    UNPUBLISHED = {DRAFT, PENDING}
    LISTED = {CONFIRMED, REVIEWED}

Docflow version:

class PostStatus(DocumentWorkflow):
    state_attr = 'status'

    draft     = WorkflowState(0, title="Draft",     description="Only owner can see it")
    pending   = WorkflowState(1, title="Pending",   description="Pending review")
    confirmed = WorkflowState(2, title="Published", description="Published")
    reviewed  = WorkflowState(3, title="Withdrawn", description="Withdrawn by owner")

    unpublished = WorkflowStateGroup([draft, pending], title="Not Published")
    listed = WorkflowStateGroup([confirmed, reviewed], title="Listed")

LabeledEnum's state values are lower level than the model that requires state values, whereas DocumentWorkflow is a wrapper around the model and thus can't supply data essential to the model itself.

However, Docflow provides two other features that LabeledEnum currently can't help with. Coaster should add support:

State queries

Consider how status queries are done with LabeledEnum (from Hasjob's JobPost model):

class JobPost(BaseMixin, db.Model):
    status = db.Column(db.Integer, nullable=False, default=POSTSTATUS.DRAFT)

    def is_draft(self):
        return self.status == POSTSTATUS.DRAFT

    def is_pending(self):
        return self.status == POSTSTATUS.PENDING

    def is_unpublished(self):
        return self.status in POSTSTATUS.UNPUBLISHED

Contrast with Docflow:

>>> wf = PostStatus(doc1)
>>> # First method, using the `is` operator
>>> wf.state is wf.draft
True
>>> # Second method, by calling the state to enquire if it's current
>>> wf.draft()
True
>>> wf.pending()
False

LabeledEnum requires a profusion of explicitly declared helper methods, while DocumentWorkflow constructs them automatically. However, DocumentWorkflow requires the document to be wrapped in the workflow first, which imposes the additional requirement of having access to the wrapper (for example in a template, which has the object it receives and everything within it, but nothing external).

It'll be nice if LabeledEnum came with a helper object that worked something like this:

class JobPost(BaseMixin, db.Model):
    status = LabeledEnumProperty(POSTSTATUS, db.Column(db.Integer, nullable=False, default=POSTSTATUS.DRAFT))
    …

>>> post = JobPost()
>>> post.status
0
>>> post.status == POSTSTATUS.DRAFT
True
>>> post.status.DRAFT
True
>>> post.status.UNPUBLISHED
True
>>> post.status.PENDING
False
>>> JobPost.query.filter(JobPost.status.DRAFT).all()  # Expands to JobPost.status == POSTSTATUS.DRAFT
>>> JobPost.query.filter(JobPost.status.UNPUBLISHED).all()  # Expands to JobPost.status.in_(POSTSTATUS.UNPUBLISHED)

State transitions

The second feature that Docflow provides is state transitions, as in this example:

class PostStatus(DocumentWorkflow):
    …
    @draft.transition(pending, None, title='Submit')
    def submit(self):
        """
        Change workflow state from draft to pending.
        """
        # Do something here
        # ...
        pass  # State will be changed automatically if we don't raise an exception

In this case the LabeledEnumProperty object can provide a transition wrapper:

class JobPost(BaseMixin, db.Model):
    status = LabeledEnumProperty(POSTSTATUS, db.Column(db.Integer, nullable=False, default=POSTSTATUS.DRAFT))

    @status.transition(POSTSTATUS.DRAFT, POSTSTATUS.PENDING)
    def submit(self):
        """
        Change status from draft to pending
        """
        self.datetime = func.utcnow()
@jace
Copy link
Member Author

jace commented Oct 30, 2017

Wrapping the SQLAlchemy column will introduce non-trivial post-processing of the class, as required by annotations (#138). We could instead take the hybrid_property approach of separate declarations. This may allow an implementation that needs no awareness of SQLAlchemy. Syntax:

class JobPost(BaseMixin, db.Model):
    _status = db.Column('status', db.Integer, default=POSTSTATUS.DRAFT, nullable=False)
    status = LabeledEnumProperty(POSTSTATUS, _status)

@jace
Copy link
Member Author

jace commented Oct 30, 2017

Hasjob shows another nuance. A status may be a combination of the status flag and something else:

class JobPost(BaseMixin, db.Model):
    …
    def is_listed(self):
        now = datetime.utcnow()
        return (self.status in POSTSTATUS.LISTED) and (
            self.datetime > now - agelimit)

If we place such methods directly on the model, we'll have the API disparity of post.is_listed vs post.status.LISTED. It should instead be possible to add to the status flags:

class JobPost(BaseMixin, db.Model):
    _status = db.Column('status', db.Integer, default=POSTSTATUS.DRAFT, nullable=False)
    status = LabeledEnumProperty(POSTSTATUS, _status)
    status.add_state(
        'CURRENTLY_LISTED',
        POSTSTATUS.LISTED,
        lambda post: post.datetime > datetime.utcnow() - agelimit
        )

Where add_state takes the new state's name, the existing state it's based on, and a function with the additional condition that must be satisfied.

@jace
Copy link
Member Author

jace commented Oct 30, 2017

It's not clear how such added states will be used for queries. This query:

JobPost.query.filter(JobPost.status.CURRENTLY_LISTED)

Expands to:

JobPost.query.filter((JobPost.status == POSTSTATUS.CURRENTLY_LISTED, JobPost.datetime > datetime.utcnow() - agelimit))

…which is invalid syntax to filter. However, using the * operator should expand correctly:

JobPost.query.filter(*JobPost.status.CURRENTLY_LISTED)

@jace
Copy link
Member Author

jace commented Oct 30, 2017

We don't yet know how to implement this LabeledEnumProperty as Python's descriptor API will apply on direct access to the status attribute, not to sub-attributes like status.DRAFT. We may need to change the API to look like post.status().DRAFT so that status gets a copy of self or cls on which to look up status flags.

@jace
Copy link
Member Author

jace commented Oct 30, 2017

Question: which of the following looks more correct?

  1. post.status.DRAFT
  2. post.status.draft
  3. post.status.is_draft

@jace
Copy link
Member Author

jace commented Oct 30, 2017

LabeledEnumProperty is a mouthful and doesn't convey what it does. The feature should be called StatusProperty instead.

@jace
Copy link
Member Author

jace commented Nov 4, 2017

The StateProperty/StateManager implemented in #153 supports both state inspection and transitions.

It should also support transitions conditional on more than one state manager. A model may have two state attributes, tracking the activity of independent actors (an author and a reviewer, for example). A transition on one of these state attributes should be available only if the other state attribute is in a specific state.

For example, a proposal on Funnel may have two state attributes:

  1. Author submission: draft, submitted, withdrawal requested, withdrawn.
  2. Review status: not received, pending, longlisted, shortlisted, accepted, rejected, cancelled.
  • When an author submits ("draft" > "submitted"), the review status automatically transitions from "not received" to "pending".

  • While review status is still "pending", the author can transition from "submitted" to "withdrawn", without the intervening "withdrawal requested" state.

  • If review state is "longlisted", "shortlisted" or "accepted", the "withdraw transition" becomes unavailable, and a "request withdraw" transition is available instead.

We currently can do this with the mechanism of added states that have a custom function evaluating the other state attribute. Chained transitions can also be achieved by using two decorators. However, this isn't good enough:

  1. Added state lambda validators are a bit crude as is. They can't seem to decide if they belong in the model or the state manager.

  2. If the inner decorator succeeds but the outer decorator fails, one of the transitions will go through unless the database commit is rolled back, and that may not happen if an external exception handler swallows the exception and then evaluates the situation.

  3. It will not be possible to accurately inspect what transitions are available in a given state since the two decorators aren't aware of each other. (We could solve for this by (a) turning the transition decorators into class decorators, (b) having them examine if the wrapped function is also a transition decorator, and (c) if so, updating the criteria on that instead of creating a new decorator.)

@jace
Copy link
Member Author

jace commented Nov 6, 2017

Conditional states (what added states are called as of 024cd1c) need a cache assertion indicating how long it is safe to assume the condition holds true or false. This is relevant when querying the state manager for available transitions, as it may not be optimal to test the conditions each time. The "available transitions" call will happen on UI renders.

We also need reliable cache invalidation:

  1. Transitions that are no longer available shouldn't remain listed as available until the cache expires.
  2. A push-update UI will require a push notification asking for the transition to be removed. It may not call the backend for a refresh for a long time.

@jace jace mentioned this issue Nov 7, 2017
7 tasks
@jace
Copy link
Member Author

jace commented Nov 9, 2017

Transitions need to be undoable. This is not necessarily the same thing as having pairs of transitions going in both directions, but an explicit relationship between two transitions indicating one undoes the other. The original states must be preserved somewhere (where?) for the undo to happen. Signal handlers must also be notified of an undo.

Since an undo will always happen in a different request, value management for affected fields (including the status field) is out of scope for the state manager. It will instead need a helper of some sort that can retrieve prior values. Push notifications will also need a recall action. Push notifications with no actual undo support (such as sending an email) may instead need to be buffered, which the undo operation should delete.

@jace jace closed this as completed in 663a15c Nov 14, 2017
jace added a commit that referenced this issue Nov 14, 2017
@jace
Copy link
Member Author

jace commented Feb 17, 2018

StateManager has resolved the workflow layer described in #100.

@iambibhas
Copy link
Contributor

The original states must be preserved somewhere (where?)

  1. in the model, have a fields called previous_state? although no default state to assign to existing records, and need changes to all models that use state manager. doesn't sound viable.
  2. a separate model to store state history maybe? although we probably dont need the whole history of state change over time. And it'll get bloated in no time.
  3. as undos are most likely to happen during short timeframe from the first action, maybe in a caching layer? redis? but yeah, if redis crashes or restarts, no more undo.

@jace
Copy link
Member Author

jace commented Feb 23, 2018

This is the part I was hoping to build with ClassView. Since it manages the request and response, it can do state management.

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

No branches or pull requests

2 participants