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

Improvements for search test coverage #1513

Draft
wants to merge 14 commits into
base: develop
Choose a base branch
from
Draft

Conversation

Oaphi
Copy link
Member

@Oaphi Oaphi commented Jan 12, 2025

part of #1509

@Oaphi Oaphi added the area: testing Changes to testing infrastructure label Jan 12, 2025
@Oaphi Oaphi self-assigned this Jan 12, 2025
@Oaphi Oaphi force-pushed the 0valt/1509/search_tests branch from b8cc3fb to 71dc1af Compare January 12, 2025 18:57
Comment on lines 3 to 6
def accessible_posts_for(user)
(user&.is_moderator || user&.is_admin ? Post : Post.undeleted)
.qa_only.list_includes
end
Copy link
Member

Choose a reason for hiding this comment

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

Should this also check category access settings here? We've got an open security report that posts in private categories sometimes show in search; this might be an ideal place to check that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup, looks like just the place to do so - will update once I am done with basic test coverage for the helper (want to make sure there are no other surprises first)

Copy link
Member Author

Choose a reason for hiding this comment

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

Or actually sooner than that as properly testing the :category qualifier requires the check to work correctly.

Copy link
Member Author

Choose a reason for hiding this comment

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

After reading through the security report itself, I realized that it doesn't seem to be related to search, @ArtOfCode-, am I missing something? I abstracted the check for category access into accessible_categories_for just in case, but the tests show that search handles category access correctly.

Copy link
Member

Choose a reason for hiding this comment

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

That's a different report - there's another one open in the support tickets that hasn't made it to a GH advisory yet.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, got it, there is another one. Hmm, looks like that one might crop up when there is no filter by category. Will take a look

@Oaphi Oaphi linked an issue Jan 13, 2025 that may be closed by this pull request
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: testing Changes to testing infrastructure
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tracking issue for improving test coverage for search
2 participants