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

Basic Workflow: support for "stage" #2256

Open
avernet opened this Issue Jun 5, 2015 · 28 comments

Comments

3 participants
@avernet
Collaborator

avernet commented Jun 5, 2015

Use case

In a small business, anyone with the role employee can submit an expense report by filling out a form. After it has been submitted, the employee who created the expense report can can see but not edit the data (but other employees can't see the data), and anyone with the role manager can either approve or reject it.

New concept: stage

Overview

  • What a stage is:
    • The set of possible stages a given form can be in is defined by the form author in Form Builder.
    • At any given point, a form is one of the stages specified by the form author. For instance, in the above use case, the form author would define the following stage: started, submitted, approved, rejected, with started marked as the initial stage.
  • What the stage influences:
    • Different buttons can be shown at the bottom of the form depending on the current stage and the user's permissions (#1619). For instance, in the above use case, once submitted, the manager would see two buttons approve and reject.
    • Whether fields are read only or visible. For instance, in the above use case, when an expense report has been submitted, all the fields in the expense report would become read only.
  • How the stage changes:
    • When a form is created, it gets the stage marked by the form author as the initial stage.
    • Buttons can change the current stage. For instance, in the above example the approve button will change the state to approved and the reject button will change the state to rejected.

In Form Builder

Stages

Out of the box, we'll have 2 stages: started (the default), and submitted. Our submit process will change the workflow stage to submitted, which will allow us to make a distinction in the database between form data that has been just saved, but hasn't been submitted yet, and data that has been submitted.

workflow stages

Available to

The Available to section shows in the Operations tab of the Workflow dialog. Its purpose is to allow form authors to whom a given operation is available, e.g. based on the user's authentication role or the workflow stage.

Design - WF - Available to

Pre-defined operations

Design - WF - Op - Create

Design - WF - Op - View

Design - WF - Op - Delete

Custom operations

Design - WF - Op - Submit

Design - WF - Op - Save

Design - WF - Op - Approve

Implementation

  • Storage and persistence layer - On relational databases, the stage is stored in a separate column. On eXist, it is stored in the metadata.xml. For the REST API, the stage is sent off-band through headers, as done currently for say, roles and operations.
  • Common code for permissions - Common code (proxy or Scala called by all persistence implementations) can determine, based on user's group and permissions, what the requirement on the stage is. Then, the persistence implementation can enforce that constraint, e.g. adding to its WHERE clause.
  • New action to change stage - E.g. change-stage('approved').
  • fr:stage() - A new XPath function is added that returns the current stage, so the stage can be used in MIPS and in processes where XPath expressions are used.
  • Redirect to view - If the user goes to /edit and has only the right to view, then she is redirect to /view.

@ebruchez ebruchez added the Workflow label Jun 6, 2015

@avernet avernet changed the title from Basic workflow to Workflow: support for "stage" Aug 26, 2015

@avernet avernet changed the title from Workflow: support for "stage" to Support for "stage" Aug 26, 2015

@avernet avernet changed the title from Support for "stage" to Basic support for "stage" Aug 26, 2015

@avernet avernet changed the title from Basic support for "stage" to Support for "stage" Aug 27, 2015

@ebruchez ebruchez added this to the 4.11 milestone Sep 14, 2015

@Kevinhebertd

This comment has been minimized.

Kevinhebertd commented Sep 29, 2015

Looking forward to this. Keep it up

@ebruchez

This comment has been minimized.

Collaborator

ebruchez commented Sep 29, 2015

@Kevinhebertd We are working on it :)

@ebruchez

This comment has been minimized.

Collaborator

ebruchez commented Nov 10, 2015

We have done quite a bit of progress on the UI. Remaining steps:

  • UI (@ebruchez)
    • complete UI
    • hookup dialog UI to "Workflow Settings" button
    • open legacy permissions if exist, new workflow if exist
    • if legacy permissions, button to migrate to new permissions (once-only operation)
  • build (@ebruchez)
    • fix sbt build
    • fix Travis build
  • backend
    • decide how to serialize workflow config in form & implement (@ebruchez)
    • generate workflow config from existing permissions config
      • Workflow & Permissions in toolbox
    • generate permissions config from workflow config
      • NOTE: don't generate if user hasn't enabled workflow
      • Q: how exact can we be?
        • A: Couldn't we be exact, given that the database should test for the stage?
  • persistence (@avernet)
    • support for Orbeon-Workflow-Stage header
    • store short permissions into metadata column / metadata.xml
    • when writing data, update stage in metadata.xml and stage column
    • check permissions for all operations (CRUD & search & published forms)
    • upgrade db schema & document
    • document API/db changes
  • simple processes and functions
    • change-workflow-stage() action (store alongside mode & version)
    • fr:workflow-stage() function
  • buttons
    • display buttons at bottom of page
    • handle legacy properties (document that they are used only if no workflow)
  • redirect to view (see above)
  • implement demo use cases
  • document
  • present new feature on orbeon.com
@Kevinhebertd

This comment has been minimized.

Kevinhebertd commented Nov 26, 2015

Do you have an updated release date for 4.11 ?? 😃

@ebruchez

This comment has been minimized.

Collaborator

ebruchez commented Nov 26, 2015

It's not going to be in 2015, that's for sure. Shooting for sometime Q1 2016.

@ebruchez

This comment has been minimized.

Collaborator

ebruchez commented May 6, 2016

UPDATE: We have progressed on this until the end of 2015, but then decided that 2016.1 (AKA 4.11) would focus instead a number of features and fixes for customers. For 2016.2, we have decided to focus on one Form Builder usability feature.

Still, there was some work done:

  • partial creation of the UI (see also #2762) with Scala.js
  • client/server communication with uPickle over our own protocol
  • case class serialization/deserialization and store into form definition

When we pick this up, we should focus on addressing #2762 first.

@ebruchez

This comment has been minimized.

Collaborator

ebruchez commented Feb 28, 2017

Remaining UI tasks:

  • when "Define custom operations for this form" is deselected, prevent dnd of operations
  • stages CSS
    • hide Default label in cell
    • make Default column narrower
  • add Save as default op (but can be removed)
  • Q: How to have Preview button? i.e. how to rename label of predefined ops?
  • Q: Should View operation automatically show Review action? 'save', 'send', 'submit'→ same?
  • Q: Can the user remove all stages?
    • currently allowed
    • BUG: right now if we remove all, then we can't re-add, getting "xf:insert - origin node-set is empty, terminating"
    • handle defaultStageKey if allowed
  • validation
    • no duplicate operation name (whether predefined or custom)
    • non-empty operation name (easy)
    • Q: I18n of custom operation names?
    • Q: Do we allow spaces in stage names? Currently we do.
    • maybe: indicate which tab has errors? error summary?
    • anything else?
  • P2
    • support organizations (what is needed?)
    • better name for "Workflow Role"
    • readonly dropdowns: color: gray or similar
    • actions: group list of processes/buttons e.g. wizard actions together
@ebruchez

This comment has been minimized.

Collaborator

ebruchez commented Feb 28, 2017

With the current permissions settings, the picture is pretty clear: CRUD operations are enabled to a user depending on:

  • roles and organizations (known without looking at the data)
  • data ownership (either as owner/creator or group member)

The basic workflow adds to this:

  • current stage (stored alongside the data as metadata)

Now at the persistence layer permission must be known in terms of CRUD. So the idea was to generate, from a workflow configuration, a permissions configuration which can be used at the lower level.

  • For create, this is easy: we have a single built-in create operation. That operation doesn't have explicit actions, and usually doesn't have a button because you just navigate to `/new', but a "Create" button is possible like Form Builder's "New" button to create a new form definition.
  • For view, we may or may not have a button, and here as well we don't have explicit operations. We raised the question or custom-naming the button above, but this is doable.
  • For delete, same thing. Usually there won't be a button but there can be one, and the permission is built into the operation name.
  • Remains update. This is not built-in, although we discussed that maybe the "Save" operation should be present by default but removable. In many cases, the update permissions will be the result of doing a "Save" or other custom operations such as "Submit" (to workflow) or "Approve". These operations change the stage, so require the update permission.

The idea was that the update operation would be inferred from the actions, specifically processes, associated with the operation. If the button associated with the operation runs a "Save" process, then we infer update. How? Probably by looking at the process definition in the properties?

One issue with that is that:

  • standard processes can be overridden in properties
  • properties can be different in different environments, such as dev, test/staging, and prod

So we need to decide how to make this as clear as possible.

Possibly, we could infer the update permission depending on the process, but show it as a checkbox. This would clarify to the form author that the permission is set or not. Then the user could override the update permission with that checkbox (or other).

@ebruchez

This comment has been minimized.

Collaborator

ebruchez commented Feb 28, 2017

Also noting that we want to infer read permissions for any update permission.

Discussion with @avernet, we think this is ok. We make it clear to the user what the update and read permissions are in the UI at least.

Also, the idea for the persistence layer is to have enhanced permissions check:

  • use of the stage
  • support conjunctions of disjunctions
@avernet

This comment has been minimized.

Collaborator

avernet commented Mar 9, 2017

Our existing format for permissions is restricted to 2-level expressions that looks like (… AND … AND…) OR (… AND … AND…) OR…. This is enough as any multi-level expression can be flattened into 2-levels. For instance:

  a OR (b AND (c OR (d AND e)))
⇔ a OR (b AND c) OR (b AND d AND e)

We'll leave the proof thereof to the reader. However, the current format does not support subtractive rules, i.e. it doesn't support NOT. We could expand it to have a 3rd level for NOT, which would allow us to rewrite any expression to the 3 levels OR / AND / NOT, in that order. For instance:

  a OR (b AND NOT (NOT c OR (d AND NOT e)))
⇔ a OR (b AND (c AND NOT (d AND NOT e)))
⇔ a OR (b AND (c AND (NOT d OR e)))
⇔ a OR (b AND c AND (NOT d OR e))
⇔ a OR (b AND c AND NOT d) OR (b AND c AND e)

With this in mind, we can either:

  1. Add a 3rd level to the permission format, and write code converting the workflow to the permissions format, as illustrated above.
  2. Expand the permission format to allow any combination and AND, OR, and NOT.

The second option would make the conversion from the workflow format to the permissions format much simpler, but makes other algorithms more complex, e.g. to determine if a user has a access to a certain operation only based on their role. With this, my current preference is for option 2.

@avernet

This comment has been minimized.

Collaborator

avernet commented Mar 9, 2017

The pre-defined operations we have in the dialog ("Create", "View", and "Delete"), correspond to built-in operations available from the Form Builder summary page. We drive other pre-defined operations from those 3, e.g. View ⇒ PDF, Create + View ⇒ Duplicate.

Now, say we have a duplicate process (once #1874 is implemented), and users define a custom operation that runs the duplicate process. Per the logic we use for Update, if users can run a operation that runs a process (duplicate) that requires Create and View, they should have the permission to Create and View. But:

  • Is this going to be reflected in the UI, i.e. the Available To for the custom Duplicate operation will be add to the Available To under Create and View? If so, what happens when Available To for the custom Duplicate operation is changed?
  • What if a form author wants certain users to be able to duplicate but not to create? Having duplicate ⇒ create would prevent this.
  • We already have this problem: a form author can't say that certain users can only create, and have a Save button on that form, as the second time they press Save we require the Update permission.
  • This might happen in other scenarios where a process requires to update data, but we don't want the user to be able to load an /edit/123 page.
  • What if form authors want to control more finely when certain operations are available, and not have this driven just by Create, View, and Delete. Say, a form author might want the PDF button to be available only when the form reached a certain stage, which calls for form authors having a PDF operation for which they can control the Available To.

It seems to me that we're having those problems because we try to reduce the permissions to run increasingly more complex operations (and even, with processes, arbitrarily complex operations), to 4 booleans Create-Read-Update-Delete. And for built-in operations, like Duplicate, PDF, and maybe more in the future, we're trying to go from a combination of the 4 booleans to the operation itself.

This is not unlike a web app with complex permissions trying to also enforce permissions at the database level, mapping application-level permissions to a coarser database-level permissions, which in general isn't being done.

I suggest we should:

  • Have permissions checked by the caller of the persistence API, not the implementation of the persistence API.
  • Still have some information about the user and the permissions provided to the search API, so it can return only data users can potentially act on.

This would have the following:

  • Drawbacks
    • If the persistence API was accessible to users by mistake, data would be less protected. But note that this already shouldn't be done today, as for instance the persistence API won't enforce all the constraints enforced by a form.
  • Benefits
    • Solve all of the above problems.
    • Make it easier for 3rd-parties to implement the persistence API.

Concretely, we would:

  • Write a conversion from permissions ADT → workflow operations ADT (but not the other way around).
  • Change our current permissions code to use the operations ADT, instead of the permissions ADT.
  • Move calls to that code to the caller of the persistence API.
@avernet

This comment has been minimized.

Collaborator

avernet commented Mar 9, 2017

For the purpose of this discussion, let's assume we go for the change suggested in the previous comment. I have 3 questions relative to the operations:

  1. What operations should be there by default?
  2. Do we really need to have special operations that are always there, and that users can't remove?
  3. Is it reasonable to conflate special operations with "their" button?

To the first question:

  • We want to have reasonable defaults for a new form, which as a starting point are equivalent to what we do if custom operations are not enabled.
  • We should add an Edit operation, available to anyone. Like Create and View, this wouldn't have customizable actions. Edit doesn't have the checkbox "Show button on detail page".
  • We should add a Save operation, available to Anyone, that runs the save-final process.

To the second question:

  • The operations Create, View, and Edit are special, in the sense that they are not tied to a process; they are accessed by loading a given URL instead, or in addition to, clicking on a button. (Delete currently isn't running a process, but could easily do so.)
  • If we remove one of those special operations, say View (which might not even be there by default), how do we add it? I can't think of a simple UI, so it is maybe better to keep them always there for now.

To the third question:

  • Conflating special operations with "their" button poses 3 problems:
    1. One of consistency: for other operations what we show in the tab is the name of the button, but for View we show View, while the button is named Review; for Create we show Create, while the button is named New.
    2. Form authors might want to have a different Available To for the button, for instance so anyone can go to /new, but only clerks who do lots of data entry have the New button.
    3. Less importantly, in general the order of the tabs reflect the order of the buttons; what should we do for operations without a button? Right now, they can be moved anywhere, which is a bit strange since in that case their position has no significance.
  • We could solve all 3 problems by:
    1. Introducing review and new processes.
    2. For Create and View remove the Show button on detail page checkbox, remove the Label section, and if form authors want to have a button, then can create a separate operations that runs the relevant process.
    3. In the list of operations, Create, View, and Edit would always come first, and wouldn't be moveable.
    4. As discussed, the Available To for the Create and New operations, or View and Review operations could be different. However, it would be an error if the Available To for New (resp. Review) wasn't a subset of the Available To for Create (resp. View). In the future we could check that, and warn form authors if we detect such a condition.
@ebruchez

This comment has been minimized.

Collaborator

ebruchez commented Mar 9, 2017

Edit doesn't have the checkbox "Show button on detail page".

What if you are on the review page, and want to go back to edit?

@avernet

This comment has been minimized.

Collaborator

avernet commented Mar 10, 2017

Other smaller changes to discuss and maybe implement:

  • What is under Available To isn't always user-related, e.g. the workflow stage, so:
    • Available ToAvailable
    • AnyoneAlways
    • No oneNever
    • Users matching the following rule(Any of | All of) the following rules are satisfied
    • Authentication roleUser authentication role
    • Workflow roleUser workflow role
  • Add a condition for the "mode", i.e. add Form shown in and dropdown with view mode, edit mode (I don't like "mode", can we do better?)
    • This is only added to operations that have a button, not the special Create, View, and Edit operations
  • Minor: in the Label section, move the dropdown and the input below the radio, as I am always thinking the the dropdown only applies of Custom is selected
@avernet

This comment has been minimized.

Collaborator

avernet commented Mar 10, 2017

What if you are on the review page, and want to go back to edit?

This is covered by the second item in my previous comment. We need a general mechanism here, in particular for the case where we have a "fill out form → review → submit" process, in which case "submit" should only show on the "review page". (And we can leverage the stage here, so it doesn't show when users go to /new.)

@avernet avernet added the Top RFE label Mar 22, 2017

@ebruchez ebruchez modified the milestones: 2017.2, 2017.1 Apr 10, 2017

@avernet

This comment has been minimized.

Collaborator

avernet commented Apr 18, 2017

@avernet

This comment has been minimized.

Collaborator

avernet commented Apr 18, 2017

Thinking about it, the term "perspective" seems to be fitting as in typical use / in the context of a workflow:

  • What you can see / do can depend on:
    • you: where you are / your role, etc;
    • the object itself: its position, location, etc / the stage, ownership, etc.
  • You may (or may not) be able to have multiple perspectives depending on constraints. In other words, what you can see / do is the sum of the what you can see / do in each perspective you are able to hold.
@avernet

This comment has been minimized.

Collaborator

avernet commented Apr 18, 2017

I updated the mockups above to make the checkbox "Fill new form" readonly if the constraints contain something that depends on the data (e.g. stage).

@ebruchez ebruchez added this to Backlog in Orbeon Forms 2017.2 Jun 9, 2017

@ebruchez ebruchez removed this from To Review in Orbeon Forms 2017.2 Oct 31, 2017

@ebruchez ebruchez removed this from the 2017.2 milestone Oct 31, 2017

@avernet

This comment has been minimized.

Collaborator

avernet commented Nov 16, 2017

@ebruchez ebruchez added this to To Review in Orbeon Forms 2018.1 Dec 1, 2017

@avernet

This comment has been minimized.

Collaborator

avernet commented Jan 15, 2018

We have a new "perspective-based" design from 2017-04, and no significant work has been done to update the UI per this new design since then. Next steps:

  • Alex to review new "perspective-based" design and see if it still makes sense.
  • Erik and Alex to review new "perspective-based" design (scheduled for 2018-01-18).
@ebruchez

This comment has been minimized.

Collaborator

ebruchez commented Jan 31, 2018

Possible next steps:

  • update UI to match new perspective-based design (#3463)
  • implement persistent changes
  • function to migrate permissions to workflow
@avernet

This comment has been minimized.

Collaborator

avernet commented Mar 22, 2018

  • As a first step towards moving where we check permissions (from the implementation of the persistence to Form Runner), we want to clarify the situations where in the future, when the new workflow is in place, we won't be able to always do that check at the level of the persistence API.
@avernet

This comment has been minimized.

Collaborator

avernet commented Apr 3, 2018

Consider the case of an expense report, and say that after an employee submits an expense report, the employee can update it, but not approve it, and a manager can approve it (or reject it), but not update it.

When a persistence API implementation gets a PUT, it doesn't have enough information to discriminate between an update of the initial expense report (only allowed if done by the employee) and an approval or rejection (only allowed if done by the manager). It doesn't have enough information to properly enforce the permissions.

It could reject operations that are clearly forbidden, like a POST by someone other than the employee or the manager. And this is something we could do in the future, but it has two downsides:

  • It's more work (obviously, but should nevertheless be stated).
  • It can provide a false sense of security, for instance leading one to think it is OK to expose the API to untrusted callers, say like a native application.

(It should be noted that this second point is already a problem right now: a form can implement the above scenario by hiding controls or making them readonly based on the user's role, and persistence API implementation doesn't check that.)

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