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

[asuworks]

No description provided.

[sgfost]

this branch is a little borked because of the last pr getting squashed and then reverting the requirements.txt change. Can you just cherry-pick fdfacaa?

[asuworks]

Thanks, Scott!

[sgfost]

sure, I believe Allen had a comment about the spread thing on the old commit. I don't think its super unreadable personally but I guess

let opts = [2,3,4];
if (hasQuery()) opts = [1, ...opts] ;

is slightly more readable

[asuworks]

sure, I believe Allen had a comment about the spread thing on the old commit. I don't think its super unreadable personally but I guess

let opts = [2,3,4];
if (hasQuery()) opts = [1, ...opts] ;

is slightly more readable

oh, did I kill it??

You mean here?

 ...(hasQueryParam("query") ? [{ value: "relevance", label: "Relevance" }] : []),

[sgfost]

yes

[alee]

LGTM, it's fine to leave the spread operator if you all prefer, it is a bit more concise

[alee]

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)

[asuworks]

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.

[asuworks]

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

[alee]

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"

[asuworks]

agree!

[alee]

closing in favor of up-to-date branch #770