signebedi / libreforms-fastapi

FastAPI implementation of the libreForms spec
GNU Affero General Public License v3.0
1 stars 1 forks source link

Implement approvals using `form_stages` construct within `__config__` #62

Closed signebedi closed 1 week ago

signebedi commented 5 months ago

The objective is to support arbitrary, group-based approval chains (eg. on submission, form X gets routed to group Y for approval, and then to group Z; if any reviewer returns the form, it goes back to the original creator). We may also be able to add assigned roles - the idea being that a proponent can identify an approver, and/or an organizational unit can identify specific individuals to play certain roles. These are general thoughts on the routing of forms for approval, disapproval, and return.

The question then becomes: are we measuring this the right way? In a manner that is sufficient? We have three placeholder fields in the document database metadata fields:

  1. _approval_signature
  2. _approved
  3. _approved_by

These are written to using the api approve form route, see #58. We also have a planned "approval chains" table to the relational data model, see #39. This should be able to measure the three kinds of situations identified in the beginning.

signebedi commented 5 months ago

One question is how to represent multiple stages of review in the document database model. I think that we can set it so that only one signature (the most recent) shows up. Older signatures will still show up in the journal.

signebedi commented 5 months ago

Ok, as I think through things, it may make sense to use the following fields in the document database metadata section:

_latest_approval_signature: str (encrypted string using app secret key?) _approval_status: str _approved_by: list _current_approval_stage:str

The _approval_status should be an enum eg. of "Submitted for approval", "Currently being reviewed", or "Approved" / "Denied" / "Returned" + substantive reviewer comments.

The _approval_status should map perfectly to a corresponding named stage in the ApprovalChains table, and the _approved_by should be a list that gets appended with approver names.

signebedi commented 5 months ago

Additionally, the ApprovalChains to define behavior on Denial and behavior on Return. For example, denial from some arbitrary stage in the chain COULD send the form back to the initiator, wiping out past approvals. Or, it could simply send it back to the last stage. These should all be behavior that can be defined in the ApprovalChains table.

So the relational data model might look something like:

  1. ID: A unique identifier for each approval chain configuration (Primary Key).
  2. Name: A descriptive name for the approval chain (e.g., "Expense Report Approval").
  3. Stage: A named stage in the approval process (e.g., "Department Review").
  4. Sequence: An integer representing the order of this stage within the approval process. This should be an int, I think, but might be redundant with Stage?
  5. ApproverRole: The role or group required to approve at this stage. Groups are straightforward. But what if you want a specific reviewal by users based on defined roles / relationships to the initiating user?
  6. OnApprove: Action or reference to the next stage upon approval (could be the next Sequence number or a specific stage name).
  7. OnDeny: Defines the action taken when a denial occurs at this stage (e.g., "Reset", "Return to Previous", specific stage name/ID).
  8. OnReturn: Defines the action taken when a document is returned for revision (e.g., "Return to Initiator", "Return to Previous", specific stage name/ID).
  9. CommentsRequired: A boolean indicating whether comments are required for approval, denial, or return actions.

The OnDeny and OnReturn fields should be flexible to accommodate different processes. For example, some documents may need to go back several stages for a comprehensive review, while others might only require minor revisions and thus, a step back or direct feedback to the initiator would suffice.

*** We should add a generate_approval_chain (form_name) factory func / table method that returns some kind of representation of the approval chain for a given form.

*** How do we represent multiple chains for a given form, eg. when the initiator is from Group 1 vs. Group 2? Maybe it makes sense to add an AppliesTo or AppliesWhen condition to the data model, which defaults to ALL / ALWAYS, but can also define more scoped application eg. to a list of groups (so, maybe a nullable foreign key > Group.name that, when null, applies to all users...)?

This also raises a consideration as to how to track approval history in the document data model. Is the journal tracking method sufficient? Or should we add eg. an _approval_history field. And how will actions taken based on the OnDeny and OnReturn configurations will be recorded in the document's _approval_history? We will want to capture approvals, sure, but also the denials and returns, along with any comments provided, to maintain a complete audit trail.

This also places pretty significant pressure on the application logic to synchronize approval status with the relational database. What if there is a change to the ApprovalChain by an admin? We need to think through that behavior. Does it render previously approved forms "unapproved"? (bad idea, from a bureaucratic perspective!) Is there a background sync process to ensure changes in the approval chain (especially denials and returns) properly update the _approval_status and potentially _current_approval_stage in the document's metadata? The bottom line is that we need to make sure that the document's status always reflects its actual position in the approval process, unless it has already been fully approved.

signebedi commented 5 months ago

As I think through things, I am starting to wonder whether we should just represent all approvals and signatures as "signatures", and then user the ApprovalChains (SignatureChains?) table to represent ordering of signatures on a form / doc in order to complete it. This shifts the process from a linear / sequential one, to one that has the possibility of being more flat and simultaneous in the manner of signature/approval aggregation. Just a thought, which needs to be processed a little more before we move to execute.

The classic example is mortgage paperwork, and contracts more generally, which need multiple signatures from parties on equal footing.

A minor problem is that this approach will simplify data structures but place greater pressure on application logic to deconflict / reconstruct...

signebedi commented 4 months ago

With the introduction of user-to-user relationships in #173, we can shift how we think about the approval table. For example, we can now make the "approval type" one of an enumerated list of options including "static", "group_based", and "relationship_based". We should also revise how signatures are stored in the form data - read: consolidate signatures into a single object - before we proceed.

signebedi commented 4 months ago

PXL_20240416_155731319

This captures much of my thinking at this point. The signatures field becomes a list of tuples containing the signature, username, timestamp, and a role_id. The role_id links to the unique identifier of a SignatureChains table entry. We also capture signatures by the form owner using this method, meaning that we get rid of the sign_own group privilege.

Then, we modify the RESTful API to include two validation routes: validate_all/{form_name}/{document_id} and validate_one/{form_name}/{document_id}/{role_id}...

signebedi commented 4 months ago

Right now, we've devised a signature roles table, but we are not actually tracking signatures required as part of the data model. Doing so would make "pushes" to the next level of review less computationally costly and complex. But, we would also more closely embed the document database with the relational database at the expense of separation of concerns principles and, in the long-run, maintainability.

Part of the question we need to ask here is what the scalability of an application like this is intended to be. Thousands of users? Tens of thousands? My hunch is that a maintainable code base for a simple form management app for small organizations is the way to go. On those grounds, then, we would prioritize maintainable, if somewhat inefficient, code.

signebedi commented 1 month ago

Now that we've added the __config__ to the form config in #315, we have an option to represent this in the form config itself. This comes with a number of tradeoffs. However, the advantage is that is the more intuitive place to put it, and requires the least custom training.

form_name:
  __config__:
    form_stages:
      initial_stage:
        label: some stage name
        action: approval
        initial_stage: true
        required: true
        method: group
        target: group_name
        on_approve: send_to_some_stage_name
        on_deny: send_to_some_stage_name
        on_pushback: send_to_some_stage_name

Supported actions include sign, approve, approved, and denied. These are rather akin to "gateways" in the sense that they describe a step through which the form will be processed, with "approved" and "denied" being terminal stages.... Alas, this leaves much to be desired, but it is a start. The stage name should be computer-friendly, but the label can override this - the label is what the end user will see.

signebedi commented 1 month ago

How do we need to represent this in the form config?

"metadata": {
  "form_stage": "current_stage_name",
  "signatures": [
    (signature, username, stage_name, timestamp),
    (signature, username, stage_name, timestamp),
  ]
}

I need to encode these tuples in json, See https://stackoverflow.com/a/715601.

signebedi commented 2 weeks ago

We need to fully implement the form approval API and UI routes. It's not clear which of those are within scope for this issue ... we ought to set some guardrails, but that might be a vanity this late in the game, at least as far as it relates to the implementation of an approval process. We have removed the "Sign Form" option from the read_one UI and added a boilerplate UI page to show all the forms needing review by the current user via a datatable, as well as a route that extends read_one to include an approval interface. This is clunky because it retains all of the dropdown options of the read_one, but saves us from having two separate files with essentially the same information. We can probably solve this down the road by creating an upstream jinja2 blueprint that read_one and individual review and approval pages inherit from, but which allows us eg. exclude the drop-down options from the latter....

We also need to implement checking on the review and approval individual page to ensure that it is timely and within scope for the current user's approval, or else return an empty content section (in the UI) or 404 (from the server side ... not recommended because it increases server/client coupling... see #329).

signebedi commented 2 weeks ago

Add parameter-based selective cache invalidation This is necessary in order to have any semblance of useful caching as it relates to stored lists of documents at each stage.

signebedi commented 2 weeks ago

Implement relationship-based form approval Currently, there are two ways to approve forms: using static and group based approval. These are straightforward enough to implement. Relationship-based approval is highly complex because of the reciprocal nature of relationships and because of how arbitrary they can be. This is a point in the application where the document and relational databases intermix, which is always a tricky business but especially so when there are additional layers of complexity (relationships) added on top of the intermixed concerns of the two data stores.

signebedi commented 2 weeks ago

Add form_stage instruction validation We've settled on using the following rules to specify form stages:

form_name:
  __config__:
    form_stages:
      initial_stage:
        label: some stage name
        action: approval
        initial_stage: true
        required: true
        method: group
        target: group_name
        on_approve: send_to_some_stage_name
        on_deny: send_to_some_stage_name
        on_pushback: send_to_some_stage_name

Some of these can have default behavior (like required); most fields will need to be specified in order for the logic to work; some will need to have their types validated; others still will need to have their values validated: for example, on_approve, on_deny, and on_pushback all need to point to another, valid stage within the same form config...

signebedi commented 2 weeks ago

Re-implement unsign / remove signature API route Adding an approval process necessitated the temporary deprecation of the unsign API route. This is a note to re-implement some API method for removing a signature from a document.

signebedi commented 2 weeks ago

Figure out how to handle pushbacks When we implement the approval process, we don't have a way to handle pushbacks in the sense that, with denial and approvals, there are obvious terminal stages, or stages back to which a document can be sent. Whereas, for pushback, the theoretical intermediary stage that it needs to be sent back to would not entail the original user "approving" their form again. So, I suppose the trigger events of on_approve, on_disapprove, and on_pushback are insufficient for such cases, as they just require resubmission, presumably with edits. Maybe: we can send the document to the same stage, but provide instructions as a message? This likely means that signatures and messages need to be stored in the approval interface for a form, so others don't approve while another pushes back, eg. for group-based approval where multiple people can approve a document... so, I think a discrete pushback behavior would be needed here... maybe a bool config like "edit_and_resubmit" that sends back to the initial_stage once completed.

signebedi commented 1 week ago

Add support for reviewer comments In the form metadata, add reviewer comments. These need not be part of the signature - especially since comments will usually be given when approval has been withheld... Further, they need not be retained, since the journal will store past comments for us. As such, a reviewer_comments that can be easily re-written & cleared on the next approval stage will probably do the trick. We will want to link these ostensibly to some type of email mechanism, too..

signebedi commented 1 week ago

Add stage_label and badge_color override option to form_stages This way end users can specify how they want arbitrary form_stage badges to appear. For example, it can display a user-friendly title, and use a badge color by class, such as those described here: https://bootswatch.com/darkly. So: Primary Secondary Success Danger Warning Info Light Dark.

signebedi commented 2 days ago

Allow admins to set a form submission's stage manually This is a bit tricky, but there are situations where admins should be able to manually set a form's stage based on those available for that specific form. Obviously, setting a form to approved manually will not come with any signatures... so this is more of a fix-in-place tool than a component of a sustainable workflow.