providing a way to disable message content being logged#1786
Conversation
|
Thanks for the contribution! Before we can merge this, we need @Parama92 to sign the Salesforce Inc. Contributor License Agreement. |
filmaj
left a comment
There was a problem hiding this comment.
Thanks for the PR! I left a comment about documenting the new option in WebClientOptions.
We will also require a unit test for this option before we can accept this PR, please 🙇 🙏
|
Thanks for the feedback! I have addressed the comments and have re-requested a review |
filmaj
left a comment
There was a problem hiding this comment.
Looking great! Very close, all I ask is that we mock out Slack API responses in the tests that you added. I provided details in my comment in-line.
Thanks again for working on this!
| }); | ||
| }); | ||
|
|
||
| describe('has an option to suppress request error from Axios', () => { |
There was a problem hiding this comment.
Thanks for adding these tests!
One thing we should do is ensure we mock out any calls to the slack.com API. Right now, since your tests are actually invoking e.g. conversations.list() method, this is issuing an HTTP request to slack.com/api. Not great, as we are a) unnecessarily creating traffic for Slack and b) have less control over the outcome of our tests. What if, during a test run, the Slack API goes down? Should the tests fail because of some service degradation? Probably not.
To handle this, we use the nock module to mock out API calls. Take a look at the nock setup and use on lines 150-175 of this file. Using nock, we can finely control a 'fake' response from the Slack API. This way, our tests will avoid both problems a) and b) above!
Additionally, since you have total control over the "Slack API" response using nock, you can not only test for the presence (and absence) of the original property in the test, you can even make sure that its contents are what you expect (since you control the API response).
There was a problem hiding this comment.
Ah! I understand. That does make sense.
I appreciate you taking the time to review this, share your thoughts, and explain the importance of each change. I apologize for any misunderstanding or oversight in my initial attempts.
There was a problem hiding this comment.
No worries at all! Zero judgment on our side, we appreciate you taking the time to send the PR and the improvements; the least we could do is help guide you along in the journey as we try to keep the quality and testing level of the project high working together 😄
filmaj
left a comment
There was a problem hiding this comment.
The code looks good to me, let's just make sure the linting and tests pass. On your machine locally, from the web-api directory, try running npm it to install the dependencies as well as run the web-api tests. It seems like a few lint rules failed (spacing and other style considerations).
|
I have addressed the issues that caused the linter to fail. It is running fine on my local machine now. Do you mind running the workflow once again? |
|
Hmm, seems like the tests are still failing. It looks to me like something is blocking the testing process; see the CI output here. I can also reproduce this behaviour locally on my macbook pro using node v20. |
|
I believe what is happening is that, because in the tests we return an error / a non-200 HTTP response, As per our documentation on retries, try setting this option in the tests on WebClient to turn off retries: |
|
It's also a good idea to run |
|
Hmm, makes sense. Strange that it seemed to work fine on my local system earlier. Hopeful that this time it will work out fine! 🤞 |
filmaj
left a comment
There was a problem hiding this comment.
Woohoo! Thank you so much for your contribution!
Summary
In this PR, we aim to prevent the logging of message content when a request error occurs while attempting to invoke any of Slack's web APIs.
It solves this issue.
The problem at a glace -
The Error type -
WebAPIRequestError, contains a property calledoriginalwhich holds the raw error received fromaxios.The
configproperty within thisoriginalis a part of the error data received fromaxios.This
configproperty might in turn hold adataproperty, which is the "message" we are trying to opt-out of showing.Tests conducted -
Error log with the opt-out config -
Note: The
originalproperty is missing from the Error object.Error log without adding the config -
Note: The
originalproperty is present in the Error object along with all the details of the request.Requirements