Hyrax: Workflows should be Admin Set-specific

[mjgiarlo]

The use case here is that installations (such as ours at Stanford but also @scholarworks's @vantuyls's and @cam156's?) want to use the one-step-mediated-deposit workflow in multiple Admin Sets, but we do not want workflow roles shared between Admin Sets.

Discussed the implementation with @jcoyne and @jeremyf on 1/25 and we think this means:

• [x] Workflows are not shared between Admin Sets -- no workflows exist outside the context of Admin Sets. This can be enforced by PermissionTemplate has_many Workflows; The PermissionTemplate should also have one active Workflow.
• [x] Each Admin Set has_many workflows
• [x] Each Admin Set can only have one active workflow at a given time.
• [x] Whenever an Admin Set is created, n workflows are imported (where n = the number of workflow templates) from the available JSON templates and associated with the Admin Set
  - [ ] We may need to be careful not to allow users to change an Admin Set's active workflow if there are items moving through the current workflow. (Is this a problem already? We should verify this before entangling that with this issue.)

[carolyncole]

@mjgiarlo I wonder in @acoburn's work with IDs vs names would solve this.

[carolyncole]

@acoburn sorry wrong auto correct for @acollier.

[mjgiarlo]

@cam156 This work by @scholarworks may be a part of this solution, but before diving into figuring out the right implementation I'd like to verify this is a shared use case.

[scholarworks]

@mjgiarlo @cam156 - This is quite true. If I have workflows assigned for one admin set and a user that is allowed to submit there, it is likely that they are not allowed to submit to any other (or at least some) admin sets. So the segregation of workflow and admin_set isn't sufficient for those use cases.

[jeremyf]

@mjgiarlo @vantuyls @cam156 @hannahfrost

As it is currently constructed, there is table for Sipity::Role. These are defined for the entire "site". However, for a Role to be available for a Workflow, there must be an entry in Sipity::WorkflowRole.

People are assigned (via Sipity::WorkflowResponsibility] to the workflow (or to a specific entity in the workflow via Sipity::EntitySpecificWorkflow).

So I believe, the Sipity::Workflow implementation delivers on your requirement.

[jcoyne]

I think the existing back end could work, I think it's the front end that needs help. We'd need to switch to using a separate workflow for each admin set, with each workflow having the same states-actions, but with different WorkflowResponsibilities.

[mjgiarlo]

Thanks, all.

[hannahfrost]

@mjgiarlo @jcoyne Are more/different tickets needed for this work?

[mjgiarlo]

@hannahfrost I wonder if @jcoyne's last comment implies we need to make this "design needed." Also tagging @ggeisler so this is on his radar.

[ggeisler]

Based on what I see related to workflows in the current code and what we've already done with admin sets, I agree we need some additional consideration of how things interact. Is there some existing documentation with use cases or other concrete description of typical workflows and what steps and roles might be involved? From a UX point of view I'm not really interested in the code underlying things, just need to better understand the details of workflows in a realistic context.

[ggeisler]

Below are proposed updates that I hope address the goals mentioned in the discussion above. The updates are pretty minor but given we've agreed (on Slack) that it makes sense for the UI to only support a couple of the most basic workflows in the near term, I think this might be a viable approach. That said, I feel slightly in the dark about workflows so I'm happy to iterate on this if I'm missing something.

• Remove the Workflows heading from the Configuration section of the Admin sidebar, and remove the associated Roles page. The Configuration > Review page is still viable, but see https://github.com/projecthydra-labs/hyku/issues/634 for the suggestion that it be moved to the Tasks section of the admin sidebar.

• Add a Reviewers section to the Participants tab of the Admin Set page. Below is the original mockup with the section that needs to be added outlined in red. The current Participants page implements most of the mockup, just not the Reviewers section.

• Add the Reviewer role to the "Select a role..." select menus at the top of the Participants page.

• For extra credit (since it's not related to the main goal of this ticket), it would be great to add a conditional for the tables within each Managers, Reviewers, etc. section such that:

• If at least one user or group has been added to a section, the table is displayed as in the mockup (and as in the current code)

• If there are no users or groups added to a section, instead of the displaying the table, display the text:
  > _No reviewers have been added to this administrative set._

where "reviewers" is obviously the appropriate role for that section. That way in the zero users/groups case we don't show just a table heading with no table content, as we currently do.

[ggeisler]

Something that occurred to me after posting my previous comment: If an admin set is configured to use a workflow that doesn't require a review step (e.g. the current default workflow), should we still be able to add Reviewers, and do they still have reviewer privileges (remove files from the work, etc.)?

I assume the answer is yes, but if so, we might want to change the wording under the Reviewers section heading to something less workflow-specific, such as:

Reviewers can approve or reject works submitted to this set, edit work metadata, and add to or remove files from a work.

[mjgiarlo]

@ggeisler :speech_balloon:

If an admin set is configured to use a workflow that doesn't require a review step (e.g. the current default workflow), should we still be able to add Reviewers, and do they still have reviewer privileges (remove files from the work, etc.)?

Good question. I'm not sure we should, given that Hyrax doesn't have a well-defined role for reviewers outside of the workflow context. For now, until we have time to consider adding a new Hyrax role, I'm inclined to say that if an AS is configured with a workflow that lacks the approving role, the Participants tab should not allow adding Reviewers. (We can still display that block in the UI, with a helpful message about Reviewers not being available with the default (0-step) workflow.)

Tagging @jcoyne, @atz, and @hannahfrost for their thoughts on this too.

[hannahfrost]

@ggeisler Thanks for mocking this up. It looks good to me. I also agree with @mjgiarlo: it should not be possible to add reviewers if the review workflow has not been configured for this admin set.

[ggeisler]

Okay, @hannahfrost and @mjgiarlo, sounds fine to me. In that case, we can still use the text under the Reviewers section heading in the mockup above for the case when the selected workflow does include a review step.

When the selected workflow does not include a review step, I suggest we show the Reviewers section heading and below it use this text (or something similar) instead of what's in the mockup:

_To add reviewers, you must configure this administrative set to use a workflow that includes a review step._

Also, if the configured workflow does not include a review step, the Reviewer role should not be included in the two "Select a role..." select menus at the top of the page.

What happens in this scenario: a review workflow is configured, reviewers are added, and then the workflow is changed to a non-review workflow? Are we okay with just removing any existing reviewers from the Participants page? That might be okay if we assume that people rarely enter more than a few reviewers per admin set. Otherwise, we might want to display a confirmation dialog if the user tries to switch from a review workflow to a non-review workflow ("Any reviewers currently assigned to this administrative set will be removed").

[mjgiarlo]

@ggeisler 💬

What happens in this scenario: a review workflow is configured, reviewers are added, and then the workflow is changed to a non-review workflow? Are we okay with just removing any existing reviewers from the Participants page? That might be okay if we assume that people rarely enter more than a few reviewers per admin set. Otherwise, we might want to display a confirmation dialog if the user tries to switch from a review workflow to a non-review workflow ("Any reviewers currently assigned to this administrative will be removed").

⬆️ @hannahfrost @vantuyls @scholarworks @jeremyf @jcoyne ❓

[hannahfrost]

@ggeisler @mjgiarlo +1 to a confirmation dialog.

I also see this as a good opportunity to kick off a notification ("You have been removed as a reviewer on the Foo collection")

[ggeisler]

That notification could probably also use a "... because the Foo collection no longer uses a workflow with a review step" clause, so that the recipient doesn't think they've been demoted from some unknown (to them) reason.

[hannahfrost]

:+1:

I will make a ticket for notifications related to user role changes (to be worked on another day).

[mjgiarlo]

Let's talk with @jeremyf to make this actionable.

[jeremyf]

The Current Participants page is a great idea. There are some caveats.

The Current Participants should render a list of all Roles assigned to that Workflow. Then, for each Role, list the associated Person or Group. It then sounds like we need a description for each of those roles; The description could be in the Sipity::WorkflowRole (as it may be the most indicative of the intention of that role).

I suppose it could be a translation, but my experience is that translations based on database entries are going to get frustrating (unless we adopt a DB backend-layer for translations as well).

[mjgiarlo]

@jeremyf For now, I believe the group's sense is that the current participants page does not need to change since two of its three roles (depositor, reviewer) map quite neatly to the two roles the current JSON workflow templates define (depositing, approving). The work that needs to be done, though I defer to your Sipity wisdom here, is making sure to treat the JSON files as templates, creating new workflow instances based on the templates for every Admin Set so that roles new not slop over from AS workflow to AS workflow. Perhaps there's a better way to implement this, though. Might you have cycles to help us clarify an approach for this ticket? Or, better yet, to work on this ticket? :)

Cc: @jcoyne

[jeremyf]

@mjgiarlo I'm reviewing my calendar now. I have one ND ticket that I need to wrap up, then I could begin work on implementing the desired behavior (though I want to steer clear of UI work, I can implement something). In reviewing my calendar, I would likely be able to begin at weeks end.

What would be helpful is some Gherkin style stories (Given, When, Then).

[ggeisler]

@jeremyf:

The Current Participants should render a list of all Roles assigned to that Workflow. Then, for each Role, list the associated Person or Group. It then sounds like we need a description for each of those roles

Putting aside the technical details necessary to make it happen, I just want to say that I _think_ the UI mockup or so up is consistent with this. The mockup is based on the assumption we made that in the near-term we're only considering a zero-step workflow or a one-step workflow with only the Reviewer role. So the Reviewer section is added as in the mockup above because this is a one-step workflow with a reviewer role. If instead this was, say, an admin set configured with a two-step workflow with a reviewer and a "metadata approver," then I'd assume we'd show an additional section titled "Metadata Approver," similar to the other sections.

I'm only thinking about the UI so I might be missing something, but from my perspective the UI approach on the Current Participants page is compatible with @jeremyf's comment, just that for now we're limiting our concern to zero- and one-step workflows (and adding a "Managers" section that will be on the Participants page for every Admin Set, independent of the workflow configured).

[jcoyne]

The problem we need to solve is that workflow_responsibilites need to be per admin set.

One way to accomplish this is to add a relationship from sipity_workflows to admin_set_id. Thus workflows will not be shared between admin sets.

This means that each admin set has many workflows that are selectable, and it has one active workflow.

We would need to break the 1-1 relationship between templates (json files) and workflows (in the db), and load all the templates as workflows associated with an admin set when a new admin set is created.

Technically the 0-step workflow could be shared among all admin sets, because it has no roles. But we don't feel good about this as it's a special case.

Alternatively, we could add join tables on workflow_responsibility to admin_set and a join for workflows to admin_set.

[jeremyf]

@jcoyne @mjgiarlo When we say AdminSet has_many…that starts crossing odd boundaries. AdminSet is ActiveFedora::Base and Workflows are ActiveRecord::Base.

Do we mean PermissionTemplate has many workflows? I'm asking because we have this item: https://github.com/projecthydra-labs/hyrax/blob/5649762ca9bcd9cb429aaf49f04ec8d280e9a979/app/forms/hyrax/forms/permission_template_form.rb#L47-L49

Also, should there be valiadation between AdminSet and PermissionTemplate? All of this is to say "Aside from two different persistence layers, I don't have a solid grasp of the separation of AdminSet and PermissionTemplate"

[jcoyne]

@jeremyf The has_many could be conceptual, not necessarily any specific function call.

PermissionTemplate has one active workflow, and many available options to choose from. The latter is returned by PermissionTemplateForm#workflows

@jeremyf I'd say the PermissionTemplate is still being worked out a bit. It's a 1-1 relationship with AdminSet, but it holds behavior related (as opposed to model related) attributes.

[jeremyf]

@jcoyne Would it be fair to say that we could approximate this with a PermissionTemplate.has_many :workflows (as per the Rails ActiveRelation concept)? This is a extension (and perhaps stretch) of the idea that the PermissionTemplate holds behavior for an AdminSet. We could then delegate #workflows from an AdminSet to the associated permission template (I think we may need methods on AdminSet and PermissionTemplate to jump boundaries from Fedora to DB)

Thinking through the impact:

1) AdminSets could have their permission template obliterated, which would mean we lose any concept of what workflows existed.
2) When an AdminSet is created, do we immediately create a permission template?

[jcoyne]

@jeremyf I think you correctly understand the cardinality of the PermissionTemplate-Workflow relationship. I don't understand the need for an AdminSet#workflows method. I think admin_set.permission_template.workflows is sufficient.

1. AdminSets could have their permission template obliterated, which would mean we lose any concept of what workflows existed.

Yes, but if the permission template is obliterated, it's likely the workflow is also obliterated. (both live in the database). Furthermore, In this scenario, we have the correct info in Fedora to preserve the existing data. (we just don't know the rules for adding new things to the admin set)

1. When an AdminSet is created, do we immediately create a permission template?

Yes

[jeremyf]

There are changes in #305 that will be impactful on this commit. I'm bumping up against not wanting to make changes in similar places (and thus force a nasty rebase/diff activity).

[jeremyf]

I believe this also builds on PR #363

[jeremyf]

Ongoing work though I discovered when I rebase the following in Sipity::Workflow:

<<<<<<< HEAD
    has_many :permission_templates, dependent: :restrict_with_exception, class_name: 'Hyrax::PermissionTemplate'
=======
    belongs_to :permission_template, class_name: 'Hyrax::PermissionTemplate'
>>>>>>> Adding PermissionTemplate.has_many :workflows

Based on this issue (and conversation with @jcoyne) I believe belongs_to :permission_template,… is the correct relationship.

[jeremyf]

I strongly encourage that Sipity::Roles are names that are applicable across admin sets (e.g. manager). It the groups/users assigned to those roles for the workflow that vary