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

IBX-6338: Do not add SortClause\ContentId if there is sort clauses #35

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

IBX-6338: Do not add SortClause\ContentId if there is sort clauses #35

wants to merge 1 commit into from

Conversation

adriendupuis
Copy link
Contributor

Question Answer
Tickets IBX-6338
Bug fix? yes
New feature? no
BC breaks? no
Tests pass? yes/no
Doc needed? no
License GPL-2.0

Following #25, I disagree with the systematic addition of this Content ID sort clause for two distinct reasons:

  • In Elasticsearch, max_score is calculated only if _score is the only sort clause. The addition of this sort clause disable max_score, preventing to extend the search with displaying a relevance ratio score/max_score.
  • Because of this addition, SortClause/Random can't be added as a SortingDefinition. "As a reviewer, I want to shuffle the results to pick one randomly for review".

Checklist:

  • Implement tests
  • Coding standards ($ composer fix-cs)

@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Nov 17, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@adriendupuis adriendupuis changed the title IBX-6338: SearchQueryType.php: Do not add SortClause\ContentId if there is already sort clauses IBX-6338: Do not add SortClause\ContentId if there is already sort clauses Nov 17, 2023
@adriendupuis adriendupuis changed the title IBX-6338: Do not add SortClause\ContentId if there is already sort clauses IBX-6338: Do not add SortClause\ContentId if there is sort clauses Nov 17, 2023
adamwojs
adamwojs reviewed Nov 20, 2023
View reviewed changes
Copy link
Member

@adamwojs adamwojs left a comment

Choose a reason for hiding this comment

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

Arguments seems to be valid, however I believe we should add we ContentId sort clause for relevant Sorting Definitions

@adriendupuis adriendupuis mentioned this pull request Nov 21, 2023
Merged
7 tasks
@konradoboza
Copy link

@adriendupuis could you please address @adamwojs remark?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adamwojs adamwojs adamwojs left review comments

@kisztof kisztof Awaiting requested review from kisztof

Assignees

@kisztof kisztof

@ViniTou ViniTou

Labels
Investigation 🕵️
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@adriendupuis @konradoboza @adamwojs @kisztof @ViniTou