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

feat: expose auto acknowledgement flag on the function handler #2334

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

Conversation

WilliamBergamin
Copy link
Contributor

@WilliamBergamin WilliamBergamin commented Nov 13, 2024

Summary

Most changes in this PR come from approved #2283 but limit the affected behavior to function_executed events

Changes

This PR changes the way to function_executed events auto acknowledgement behavior operates. These changes remove auto acknowledgement for function_executed events and lift the logic into a middleware that is then registered to the function event listener

This allows the app.function listener to expose an autoAcknowledge flag that can be used to disable the auto ack middleware. This is required for the Dynamic Options, since functions used as dynamic inputs must complete or fail before acknowledging the request (this provides a crisp user experience).

Example

app.function('get-options', { autoAcknowledge: false }, async ({ inputs, ack, fail, complete }) => {
  try {
    const category: string[] = inputs.category as string[];
    if (category.includes('dogs')) {
      complete({
        outputs: {
          options: [
            { key: 'Poodle', value: 'value-0' },
            { key: 'Bulldog', value: 'value-1' },
            { key: 'Beagle', value: 'value-2' },
          ],
        },
      });
    } else if (category.includes('cars')) {
      complete({
        outputs: {
          options: [
            { key: 'Toyota', value: 'value-0' },
            { key: 'Ford', value: 'value-1' },
            { key: 'Jeep', value: 'value-2' },
          ],
        },
      });
    } else {
      fail({ error: `Invalid category: ${category} (valid: 'dogs', 'cars')` });
    }
  } catch (error) {
    fail({ error: `Failed to handle a function request (error: ${error})` });
  } finally {
    await ack();
  }
});

Manual tests

These changes were manually tested against

Testing

  1. Pull this branch
  2. npm install & npm run build
  3. Use npm install path/to/the/bolt-js/project to install the built package in a project with a custom step like bolt-js-custom-step-template
  4. Set the options field of the function handler to { autoAcknowledge: false }
  5. You should now be able to use the ack input parameter in your handler
  6. AutoAck behavior is now disabled you must now manually call await ack(); for this specfic handler

Feedback

I've messed around with the CustomFunction exported types I am not if these are considered "breaking changes" in this project. There may be a way to work around this.

Should there be more unit tests around these changes?

Requirements (place an x in each [ ])

@WilliamBergamin WilliamBergamin added enhancement M-T: A feature request for new functionality semver:minor labels Nov 13, 2024
@WilliamBergamin WilliamBergamin self-assigned this Nov 13, 2024
Copy link

codecov bot commented Nov 13, 2024

Codecov Report

Attention: Patch coverage is 93.90863% with 12 lines in your changes missing coverage. Please review.

Project coverage is 91.08%. Comparing base (248663d) to head (4ae983d).

Files with missing lines Patch % Lines
src/App.ts 87.35% 10 Missing and 1 partial ⚠️
src/CustomFunction.ts 98.82% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2334      +/-   ##
==========================================
- Coverage   92.49%   91.08%   -1.41%     
==========================================
  Files          36       22      -14     
  Lines        7472     6116    -1356     
  Branches      651      652       +1     
==========================================
- Hits         6911     5571    -1340     
+ Misses        553      540      -13     
+ Partials        8        5       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/App.ts Outdated
Comment on lines 1117 to 1121
} else {
// Events API requests are acknowledged right away, since there's no data expected
// Except function_executed events
await listenerArgs.ack();
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

All events that are not function_executed are automatically acknowledged, preserving the existing behavior 🥇

@WilliamBergamin WilliamBergamin marked this pull request as ready for review November 14, 2024 14:44
Copy link
Contributor

@filmaj filmaj left a comment

Choose a reason for hiding this comment

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

Overall looks good to me implementation wise!

Two things I'd like to make sure we do before I feel comfortable ✅ this PR:

  1. Let's study the code coverage report and understand why the code coverage dropped by 1.5%. Which parts of the code is now untested, and what can we do to improve that situation in this PR? What code is relevant for us to cover as part of this work?
  2. One thing we realized in our explorations during this feature is that the existing auto-acknowledgement of event is a feature that is not tested at all. Let's make sure to add tests for this. Not exactly sure where we should put those tests - maybe test/unit/App/middleware.spec.ts? Maybe one test in each of the test/unit/App/routing-*.spec.ts files? In the routing files might work: this way, for each of the event classes that bolt-js exposes a different listener registration function for, we can write a test that formalizes the expected acknowledgement behaviour. For the case of custom functions, we'd have two such tests (one for autoack:true and one for autoack:false).

src/types/events/index.ts Show resolved Hide resolved
test/types/functions.test-d.ts Show resolved Hide resolved
const args = createDummyCustomFunctionMiddlewareArgs({ callbackId: 'my_id' });
let isAck = false;
await fakeReceiver.sendEvent({
ack: async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Tiny nit: can we use sinon here instead of defining a sentinel boolean variable, etc.?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point not sure why I did not opt to use this originally

);
}

describe('App function() routing', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

What about sad path tests? Say a function_executed event comes in with a callback_id that we DO NOT have a listener for - what behaviour is expected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great point adding that in 💯

src/App.ts Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement M-T: A feature request for new functionality semver:minor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants