providing a way to disable message content being logged

[Parama92]

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 called original which holds the raw error received from axios.
The config property within this original is a part of the error data received from axios.
This config property might in turn hold a data property, which is the "message" we are trying to opt-out of showing.

Tests conducted -

1. Error log with the opt-out config -
   
   Note: The original property is missing from the Error object.

2. Error log without adding the config -
   
   Note: The original property is present in the Error object along with all the details of the request.

Requirements

• I've read and understood the Contributing Guidelines and have done my best effort to follow them.
• I've read and agree to the Code of Conduct.

[salesforce-cla]

Thanks for the contribution! Before we can merge this, we need @Parama92 to sign the Salesforce Inc. Contributor License Agreement.

[filmaj]

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 🙇 🙏

[Parama92]

Thanks for the feedback! I have addressed the comments and have re-requested a review

[filmaj]

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!

[filmaj]

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).

[Parama92]

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.

[filmaj]

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]

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).

[Parama92]

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?

[filmaj]

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.

[filmaj]

I believe what is happening is that, because in the tests we return an error / a non-200 HTTP response, web-api will by default attempt to retry issuing the request (see this default option). The default retry mechanism is "10 retries in 30 minutes" - meaning, if the Slack API returns an error result (like the "fake" API behaviour in the new tests you added), web-api will block the process in order to retry the request, blocking the process for up to 30 mins! I believe this is what is causing the tests to hang.

As per our documentation on retries, try setting this option in the tests on WebClient to turn off retries:

{
  retryConfig: { retries: 0 },
}

[filmaj]

It's also a good idea to run npm test locally before pushing any changes to make sure everything works as expected on your machine.

[Parama92]

Hmm, makes sense. Strange that it seemed to work fine on my local system earlier.
Thanks for the heads-up though! I make the necessary changes and ran npm test. I am getting an unrelated error, which didn't show up in any of the test runs here 🤔 -

Hopeful that this time it will work out fine! 🤞

[filmaj]

Woohoo! Thank you so much for your contribution!