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

Engagement Workflow v2 #3245

Open
wants to merge 19 commits into
base: develop
Choose a base branch
from
Open

Engagement Workflow v2 #3245

wants to merge 19 commits into from

Conversation

atGit2021
Copy link
Contributor

@atGit2021 atGit2021 self-assigned this Jun 18, 2024
@atGit2021
Copy link
Contributor Author

atGit2021 commented Jun 18, 2024

Todo: commented out StepHistoryToWorkflowEventsMigration file for now

@atGit2021 atGit2021 force-pushed the 0039-engagement-workflow branch from d9392c5 to 58d081c Compare June 21, 2024 18:37
@atGit2021 atGit2021 marked this pull request as ready for review June 28, 2024 20:31
@atGit2021 atGit2021 requested a review from CarsonF as a code owner June 28, 2024 20:31
@atGit2021
Copy link
Contributor Author

@CarsonF I thought I would have you review before starting to dig into the migration file.

Copy link
Member

@CarsonF CarsonF left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks really close other than a few minor things!

Drop 788f863 - we aren't versioning these access policies since they are generated.

@atGit2021 atGit2021 force-pushed the 0039-engagement-workflow branch from b1ba482 to 2234b7b Compare July 2, 2024 14:15
Copy link

github-actions bot commented Jul 2, 2024

🗞 GraphQL Summary

View schema changes
@@ -1463,8 +1463,9 @@
   status: SecuredEngagementStatus!
 
   """The last time the engagement status was modified"""
   statusModifiedAt: SecuredDateTime!
+  workflowEvents: [EngagementWorkflowEvent!]!
 }
 
 input EngagementFilters {
   language: LanguageFilters
@@ -1558,18 +1559,39 @@
   """@order 14"""
   Unapproved @deprecated(reason: "Legacy. Only used in historic data.")
 }
 
-type EngagementStatusTransition {
-  label: String!
+type EngagementWorkflowEvent {
+  at: DateTime!
+  id: ID!
+  notes: SecuredRichTextNullable!
   to: EngagementStatus!
-  type: EngagementTransitionType!
+
+  """The transition taken, null if workflow was bypassed"""
+  transition: EngagementWorkflowTransition
+  who: SecuredActor!
 }
 
-enum EngagementTransitionType {
-  Approve
-  Neutral
-  Reject
+"""
+A transition for the engagement workflow.
+
+This is not a normalized entity.
+A transition represented by its `key` can have different field values
+based on the workflow's state.
+"""
+type EngagementWorkflowTransition {
+  disabled: Boolean!
+  disabledReason: String
+
+  """
+  An local identifier for this transition.
+  It cannot be used to globally identify a transition.
+  It is passed to the transition mutation.
+  """
+  key: ID!
+  label: String!
+  to: EngagementStatus!
+  type: TransitionType!
 }
 
 type EthnoArt implements Producible & Resource {
   """Whether the requesting user can delete this resource"""
@@ -1632,8 +1654,28 @@
   name: String
   provisionalCode: String
 }
 
+input ExecuteEngagementTransitionInput {
+  """
+  Bypass the workflow, and go straight to this state.
+  `transition` is not required and ignored when using this.
+  """
+  bypassTo: EngagementStatus
+
+  """The engagement ID to transition"""
+  engagement: ID!
+
+  """Any additional user notes related to this transition"""
+  notes: RichText
+
+  """
+  The transition `key` to execute.
+  This is required unless specifying bypassing the workflow with a `step` input.
+  """
+  transition: ID
+}
+
 input ExecuteProgressReportTransitionInput {
   """Any additional user notes related to this transition"""
   notes: RichText
 
@@ -2162,8 +2204,9 @@
   status: SecuredEngagementStatus!
 
   """The last time the engagement status was modified"""
   statusModifiedAt: SecuredDateTime!
+  workflowEvents: [EngagementWorkflowEvent!]!
 }
 
 enum InternshipPosition {
   Administration
@@ -2494,8 +2537,9 @@
   status: SecuredEngagementStatus!
 
   """The last time the engagement status was modified"""
   statusModifiedAt: SecuredDateTime!
+  workflowEvents: [EngagementWorkflowEvent!]!
 }
 
 type LanguageEngagementListOutput implements PaginatedList {
   """Whether the next page exists"""
@@ -3187,8 +3231,9 @@
     Whether the item should be pinned or not. Omit to toggle the current state.
     """
     pinned: Boolean
   ): Boolean!
+  transitionEngagement(input: ExecuteEngagementTransitionInput!): Engagement!
   transitionProgressReport(input: ExecuteProgressReportTransitionInput!): ProgressReport!
   transitionProject(input: ExecuteProjectTransitionInput!): Project!
 
   """Update a budget"""
@@ -4968,8 +5013,9 @@
   educations(input: EducationListInput = {count: 25, order: ASC, page: 1, sort: "institution"}): EducationListOutput! @deprecated(reason: "Query via user instead")
 
   """Lookup an engagement by ID"""
   engagement(changeset: ID, id: ID!): Engagement!
+  engagementWorkflow: Workflow!
 
   """Look up engagements"""
   engagements(input: EngagementListInput = {count: 25, order: ASC, page: 1, sort: "createdAt"}): EngagementListOutput!
 
@@ -5556,16 +5602,16 @@
 """
 type SecuredEngagementStatus implements Secured {
   """
   Is the current user allowed to bypass transitions entirely
-  and change the status to any other status?
+  and change to any other state?
   """
   canBypassTransitions: Boolean!
   canEdit: Boolean!
   canRead: Boolean!
 
-  """The available statuses a engagement can be transitioned to."""
-  transitions: [EngagementStatusTransition!]!
+  """The transitions currently available to execute for this engagement"""
+  transitions: [EngagementWorkflowTransition!]!
   value: EngagementStatus
 }
 
 """

🚨 Breaking Changes

  • EngagementStatusTransition was removed.
  • EngagementTransitionType was removed.
  • SecuredEngagementStatus.transitions changed type from [EngagementStatusTransition!]! to [EngagementWorkflowTransition!]!.

@atGit2021 atGit2021 force-pushed the 0039-engagement-workflow branch 2 times, most recently from 0ff78f5 to abf4de5 Compare July 2, 2024 22:35
@atGit2021 atGit2021 requested a review from CarsonF July 2, 2024 22:42
@CarsonF CarsonF marked this pull request as draft July 5, 2024 22:16
@CarsonF CarsonF force-pushed the 0039-engagement-workflow branch 2 times, most recently from 3d6e58c to bb305fa Compare July 9, 2024 20:52
@CarsonF CarsonF marked this pull request as ready for review July 9, 2024 21:10
atGit2021 and others added 8 commits July 12, 2024 17:33
In general, just tried to follow suit of project workflow
Seth agrees this doesn't make sense
…late start & early end tests

Mostly, we just want the engagement status to follow the project status.
This is already being tested in engagement.e2e-spec.ts.
So I removed all of these.

I did add a couple where we think users will be directly interacting with the engagement workflow
@CarsonF CarsonF force-pushed the 0039-engagement-workflow branch from 7cdb117 to e0accee Compare July 12, 2024 22:33
readonly session: Session,
readonly input?: UpdateLanguageEngagement | UpdateInternshipEngagement,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@CarsonF I'm not sure if changing the input param to optional has further impacts but based on high level scans I think this will be ok, but let me know if there is a better way to design the event publishing for engagement workflow.
@bryanjnelson @willdch

ProjectStep.Terminated,
EngagementStatus.Terminated,
],
// [
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@CarsonF I commented out this element of the array which was causing the following error. I'm not sure how to fix this, but putting my findings below:

 should update Engagement status to match Project step when it becomes PendingTerminationApproval
    [Unauthorized, Input, Client]: This transition is not available

at the following location:
at findTransition (src/components/workflow/workflow.service.ts:159:11)

expect(lEngagementStatus.status.value).toBe(EngagementStatus.Active);
});
await controller.login();
await eng.executeByLabel('Approve');
Copy link
Contributor Author

@atGit2021 atGit2021 Jul 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@CarsonF , The executeByLabel('Approve') flows down to the method findTransition in the workflow.tester.ts file where it fails to find a transition and throws the exception, This transition is not available. I tried to debug this and the rest of the CI errors, but I'm not understanding why the transitions are not being found.

);
});
await controller.login();
await eng.executeByState('Completed');
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar exception described above is happening here.

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

Successfully merging this pull request may close these issues.

2 participants