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.

Sign up for GitHub

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

Jump to bottom

Multiple categories users fix #94

Merged
merged 4 commits into from
Aug 7, 2024
Merged

Multiple categories users fix #94

merged 4 commits into from
Aug 7, 2024

Conversation

waximabbax
Copy link
Contributor

@waximabbax waximabbax commented Aug 6, 2024 •
edited
Loading

For: #93

addingusers-.2.mp4

@waximabbax waximabbax force-pushed the multiple-categories-users-fix branch 2 times, most recently from 240adca to f23f0f6 Compare August 6, 2024 15:37
@waximabbax waximabbax changed the title WIP: Multiple categories users fix Multiple categories users fix Aug 6, 2024
@waximabbax waximabbax marked this pull request as ready for review August 6, 2024 15:47
@waximabbax waximabbax force-pushed the multiple-categories-users-fix branch from f23f0f6 to 3e93819 Compare August 6, 2024 16:30
@waximabbax waximabbax marked this pull request as draft August 6, 2024 18:07
@waximabbax waximabbax force-pushed the multiple-categories-users-fix branch 2 times, most recently from 74b57df to 6489fd1 Compare August 6, 2024 18:19
@waximabbax waximabbax marked this pull request as ready for review August 6, 2024 18:34
JeroenDeDauw
JeroenDeDauw reviewed Aug 6, 2024
View reviewed changes
Copy link
Member

@JeroenDeDauw JeroenDeDauw left a comment

Choose a reason for hiding this comment

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

A regression test would be nice

@waximabbax waximabbax force-pushed the multiple-categories-users-fix branch from 6489fd1 to aa64ed2 Compare August 6, 2024 22:55
@waximabbax
Copy link
Contributor Author

A regression test would be nice

I manually verified all the changes, and we already do have SpecialApproverCategoriesTest

https://github.com/ProfessionalWiki/PageApprovals/blob/master/tests/EntryPoints/Specials/SpecialApproverCategoriesTest.php#L16

Should we add something that SpecialApproverCategoriesTest doesn't cover?

@malberts
Copy link
Contributor

malberts commented Aug 7, 2024 •
edited
Loading

Should we add something that SpecialApproverCategoriesTest doesn't cover?

Yes, the scenario that was not working.

While this file is getting touched, SpecialApproverCategoriesTest should be renamed to SpecialManageApproversTest.

@malberts
Copy link
Contributor

malberts commented Aug 7, 2024

New test: #99

@malberts malberts merged commit 0ace169 into master Aug 7, 2024
16 checks passed
@malberts malberts deleted the multiple-categories-users-fix branch August 7, 2024 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@malberts malberts malberts left review comments

@JeroenDeDauw JeroenDeDauw JeroenDeDauw approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@waximabbax @malberts @JeroenDeDauw