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

Stop showing scheduled deletions on unrelated channels #3041

Conversation

michellemounde
Copy link
Contributor

Fixes #2312:

  1. Modify ruleMatchesChannel function to account for scheduled deletions
  2. Rename tests for increased clarity on what is being tested
  3. Create tests for scenarios that arose from rewording tests
  4. Create tests for scheduled deletions matching

? (rule.channel &&
ruleChannelMatches &&
rule.scheduledChange.channel === null) ||
(!rule.channel && rule.scheduledChange.channel === null) ||
Copy link
Contributor

Choose a reason for hiding this comment

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

This appears to work, but I'm suspicious that something subtle is off here. I was expecting rule.scheduledChange.change_type === 'delete' to be treated specially - not to have all scheduled change types have this logic applied. For change_type === update or insert the channel should be matching if either the rule or rule.scheduledChange channel matches.

Could you try to make this a bit more clear? Feel free to drop the const on scChannelMatches and write this as if/else statements if it helps.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't sure whether to query for the change type initially or just make the function work for all scheduled changes. I appreciate the feedback and I'll work on making it clear.

Copy link
Contributor

@bhearsum bhearsum left a comment

Choose a reason for hiding this comment

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

This looks good, and my local testing can't find any cases that are displaying incorrectly. Thank you!

@bhearsum bhearsum merged commit 4b6ca55 into mozilla-releng:main Nov 2, 2023
6 checks passed
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.

UI shows scheduled deletions for unrelated channels
2 participants