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

Support jobs from called activities #17

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

kevinmook
Copy link
Collaborator

What did we change?

Support jobs from called activities:
called_activity

I also updated the error messages for when the processInstanceKey doesn't match. Previously the error looked like:

     Failure/Error:
       activate_job("notify_role").
         and_complete(task_id: task_id)

       expected: 2251799813686578
            got: 2251799813686595

       (compared using ==)

Now it looks like:

     Failure/Error:
       activate_job("notify_role").
         and_complete(task_id: task_id)

       expected the job's processInstanceKey ('2251799813686630') to match the process_instance_key ('2251799813686613')

Why are we doing this?

When a bpmn uses a "call activity" type, the processInstanceKey won't match the parent's processInstanceKey. This causes the activate_job helper to fail.

This adds an argument to activate_job to indicate that the job was created by a called activity.

Another workaround would be to set validate: false, but that would also prevent other helpful validations, like ensuring the job's type is correct.

Feedback on the argument name welcome, I'm happy to change it from "called" to something else if anyone has a better suggestion.

How was it tested?

  • Specs
  • Locally
  • Staging

@kevinmook kevinmook requested review from rusterholz and tjwp March 5, 2022 21:19
Copy link
Collaborator

@rusterholz rusterholz left a comment

Choose a reason for hiding this comment

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

I love the intention here, but I'd like to see a clearer name for the new parameter -- and I think that might be best achieved if we also rename validate; see below.

@@ -21,8 +21,13 @@ def initialize(job, type:, process_instance_key:, client:, context:, validate:)
context.instance_eval do
expect(job).not_to be_nil, "expected to receive job of type '#{type}' but received none"
aggregate_failures do
expect(job.processInstanceKey).to eq(process_instance_key)
expect(job.type).to eq(type)
unless called
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think putting unless called inside if validate might be a little confusing, both in terms of code structure and also for users of this API (since validate and called are bound in a non-obvious way).

What if we were to separate and rename both validate and called into new options that spell out exactly what they do and have sensible defaults? The validate option could perhaps be presence: true, and called: false could be main_instance: true, or something like that. Then there could be separate if presence and if main_instance blocks.

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

Successfully merging this pull request may close these issues.

2 participants