Skip to content

Conversation

@AltamashShaikh
Copy link
Contributor

Description

Improves Slack ChannelID validation to allow only alphanumeric values

Issue No

QA found out that non-alphanumeric characters can be part of SlackChannelID values. PR aims to add a validation rule to allow only alphanumeric values

Steps to Replicate the Issue

  1. Try to set a slackChannelID value in ScheduleReports and CustomAlerts containing non-alphanumeric characters
  2. This will succeed
  3. Now checkout and try performing the same action and this PR should now throw error

Checklist

  • [✔] Tested locally or on demo2/demo3?
  • [✔] New test case added/updated?
  • [✔] Are all newly added texts included via translation?
  • [NA] Are text sanitized properly? (Eg use of v-text v/s v-html for vue)
  • [NA] Version bumped?

@AltamashShaikh AltamashShaikh added the Needs Review For pull requests that need a code review. label Sep 24, 2025
@AltamashShaikh AltamashShaikh requested a review from a team September 24, 2025 02:04
Copy link
Contributor

@snake14 snake14 left a comment

Choose a reason for hiding this comment

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

Looks good! 👍

I left one question.

} elseif (empty($parameters[self::SLACK_CHANNEL_ID_PARAMETER])) {
throw new \Exception(Piwik::translate('Slack_SlackChannelIdRequiredErrorMessage'));
}
$slackChannels = explode(',', (string) $parameters[self::SLACK_CHANNEL_ID_PARAMETER]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this just use the validateCustomAlertReportParameters method? It's a little confusing that this appears to allow multiple channel IDs while validateCustomAlertReportParameters doesn't.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@snake14 Files can be shared to multiple channelIDs by specifying comma separated values, but this does not work for sending a text message for Slack, hence we have 2 different checks

@AltamashShaikh AltamashShaikh merged commit a6e6774 into 5.x-dev Sep 24, 2025
15 checks passed
@AltamashShaikh AltamashShaikh deleted the improve-validation branch September 24, 2025 05:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Needs Review For pull requests that need a code review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants