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

Searchbox bugfix #489

Open
wants to merge 17 commits into
base: master
Choose a base branch
from
Open

Conversation

segir187
Copy link
Contributor

@segir187 segir187 commented Mar 18, 2025

Fixes #477. Fixes #485. Fixes #486.

Fix of three bugs related to the problemset searchbox I found while implementing #459 .

@segir187 segir187 added the bug label Mar 18, 2025
@segir187 segir187 requested a review from MasloMaslane as a code owner March 18, 2025 20:22
@@ -116,10 +116,10 @@ function init_search_selection(id) {
} else if (item.trigger !== 'problem') {
// At this point for anything other than a problem we
// want to create a search tag
const value = item.value || item.name;
const value = item.value; // IS THIS CHANGE GOOD?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previously this value defaulted to item.name if item.value was not given.

As I am pretty sure that all places in the code where item.value was previously not given I changed it to be given, is the removal of this value defaulting to item.name a good change or is it still unnecessary?

result.extend(get_nonselected_origintag_hints(query))
result.extend(get_origininfovalue_hints(query))

if settings.PROBLEM_TAGS_VISIBLE or request.user.is_superuser:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This fix only ensures that hints related to tags are not given to non-superusers. It is still possible for any user to manually filter by the tags they want by changing the URL.

Should I make it so that it is impossible for non-superusers to filter by tags in such a way if they are hidden?

Should I make it so that superusers are also unable to filter by hidden tags, as the tags still don't show up next to tasks for them when PROBLEM_TAGS_VISIBLE = False?

@@ -159,7 +159,39 @@ class TestProblemSearchHintsTags(TestCase, AssertContainsOnlyMixin):
def get_query_url(self, parameters):
return self.url + '?' + urllib.parse.urlencode(parameters)

@override_settings(LANGUAGE_CODE="en")
@override_settings(LANGUAGE_CODE="en", PROBLEM_TAGS_VISIBLE=False)
def test_search_no_hints_tags(self):
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 added this test to make sure that with PROBLEM_TAGS_VISIBLE=False the hints do not show up. Should I duplicate each test that fails with PROBLEM_TAGS_VISIBLE=False like this?

@@ -209,7 +242,7 @@ def test_search_hints_origininfo(self):
self.assertEqual(response.status_code, 200)
self.assert_contains_only(response, ['origintag', 'round', 'year'])

@override_settings(LANGUAGE_CODE="en")
@override_settings(LANGUAGE_CODE="en") # PROBLEM_TAGS_VISIBLE=True
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some tests still pass with PROBLEM_TAGS_VISIBLE=False, as they work on hint categories rather than hints themselves. Unclear on how to proceed with this.

@segir187 segir187 marked this pull request as draft March 18, 2025 20:56
@segir187 segir187 marked this pull request as ready for review March 25, 2025 21:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
1 participant