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

fix: remove Relevance when query parameter is empty for /codebases/ search #769

Closed
wants to merge 1 commit into from
Closed

fix: remove Relevance when query parameter is empty for /codebases/ search #769

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 1 addition & 4 deletions django/core/jinja_config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,10 +131,7 @@ def generate_search_form_inputs(query_params):
Returns:
list: A list of tuples representing the hidden input fields.
"""
# set default ordering to relevance, if it is not specified
search_parameters = {
"ordering": "relevance",
}
search_parameters = {}
if query_params:
# parse_qsl handles splitting and unquoting key-value pairs and generates a list of tuples
# do not include the actual query in the query parameters
Expand Down
9 changes: 5 additions & 4 deletions django/library/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -399,12 +399,13 @@ def filter_queryset(self, request, queryset, view):
criteria.update(id__in=codebases.values_list("id", flat=True))
# or we could include the PL in the query
# qs += " ".join(programming_languages)

default_ordering = "-first_published_at"
if ordering:
Copy link
Member

Choose a reason for hiding this comment

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

I think this conditional could probably be simplified also, is this equivalent?

        default_ordering = "relevance" if qs else "-first_published_at"
        criteria.update(ordering=ordering if ordering else default_ordering)

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 asked Perplexity if the approaches are equivalent. Crazy, it does execution! I have also checked the answer. It is correct.

Perplexity Response

The two approaches are not equivalent, as demonstrated by the execution results of the provided code snippets.

Approach 1:

default_ordering = "-first_published_at"
if ordering:
    criteria.update(ordering=ordering if qs else default_ordering)
else:
    # set default ordering for search when ordering is not specified
    criteria.update(ordering="relevance" if qs else default_ordering)

Execution Results:

(True, None, 'relevance')
(True, 'some_order', 'some_order')
(False, None, '-first_published_at')
(False, 'some_order', '-first_published_at')

Approach 2:

default_ordering = "relevance" if qs else "-first_published_at"
criteria.update(ordering=ordering if ordering else default_ordering)

Execution Results:

(True, None, 'relevance')
(True, 'some_order', 'some_order')
(False, None, '-first_published_at')
(False, 'some_order', 'some_order')

Key Differences:

When qs is False and ordering is 'some_order':
Approach 1 results in -first_published_at.
Approach 2 results in some_order.

This difference arises because Approach 1 checks whether ordering should be used based on the value of qs, while Approach 2 uses ordering directly if it is provided. Therefore, the logic for determining the final ordering differs between the two approaches when qs is False and an ordering is specified.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TL;DR if no query is given, the ordering should be -first_published_at and not some_order which might be present in request params

Copy link
Member

@alee alee Oct 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

That's cute that the AI can generate truth tables! I ran this a few times on perplexity and it sometimes got the wrong answer though. Also it doesn't actually answer the question of "is it the behavior we want"...

Four cases:

  1. ordering is set, qs is not - should be set to ordering, right? not the default ordering. This is when user changes the sort by ordering without specifying a query
  2. ordering is not set, but qs is - set to "relevance" by default
  3. ordering is set, qs is set - respect ordering requested by the user again
  4. ordering not set, qs is not set - use "-first_published_at"

Here's the current truth table for approach 1:

ordering qs criteria['ordering']
True True ordering
True False default_ordering
False True "relevance"
False False default_ordering

The issue is the second line here, which should respect the user-specified ordering even when qs is empty.

The code I suggested would generate something like this instead:

ordering qs default_ordering criteria['ordering']
True True "relevance" ordering
True False "-first_published_at" ordering
False True "relevance" "relevance"
False False "-first_published_at" "-first_published_at"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agree!

criteria.update(ordering=ordering)
criteria.update(ordering=ordering if qs else default_ordering)
else:
if qs:
# set default ordering for search when ordering is not specified
criteria.update(ordering="relevance")
# set default ordering for search when ordering is not specified
criteria.update(ordering="relevance" if qs else default_ordering)

return get_search_queryset(qs, queryset, tags=tags, criteria=criteria)

Expand Down
25 changes: 17 additions & 8 deletions frontend/src/apps/codebase_list.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,20 @@ import { extractDataParams } from "@/util";
const props = extractDataParams("sidebar", ["languageFacets"]);
createApp(CodebaseListSidebar, props).mount("#sidebar");

createApp(SortBy, {
sortOptions: [
{ value: "relevance", label: "Relevance" },
{ value: "-first_published_at", label: "Publish date: newest" },
{ value: "first_published_at", label: "Publish date: oldest" },
{ value: "-last_modified", label: "Recently Modified" },
],
}).mount("#sortby");
// Function to check if 'query' exists and is not empty
function hasQueryParam(param: string) {
const params = new URLSearchParams(window.location.search);
const value = params.get(param);
return value !== null && value.trim() !== "";
}

const relevanceOption = hasQueryParam("query") ? [{ value: "relevance", label: "Relevance" }] : [];

const sortOptions = [
...relevanceOption,
{ value: "-first_published_at", label: "Publish date: newest" },
{ value: "first_published_at", label: "Publish date: oldest" },
{ value: "-last_modified", label: "Recently Modified" },
];

createApp(SortBy, { sortOptions }).mount("#sortby");
2 changes: 1 addition & 1 deletion frontend/src/components/CodebaseListSidebar.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ const initialFilterValues = {
onMounted(() => {
if (props.languageFacets) {
const localLanguageFacets = { ...props.languageFacets };
console.log(localLanguageFacets);
// console.log(localLanguageFacets);
parsedLanguageFacets = Object.entries(localLanguageFacets)
.sort(([, valueA], [, valueB]) => valueB - valueA) // Sort by value in descending order
.map(([name, value]) => ({ value: name, label: `${name} (${value})` }));
Expand Down
Loading