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

Features/threshold in response #68

Merged
merged 5 commits into from
Jan 23, 2024
Merged

Conversation

gsfk
Copy link
Member

@gsfk gsfk commented Jan 10, 2024

Mention censorship threshold in beacon response. (Redmine #1918)

A short message is added to all searches with no results. The same message is given whether censorship was applied or not (the true results may be zero, or some amount within the threshold for censorship that was then changed to zero). Mentioning it only when censorship is applied would be a data leak.

Despite the text in the ticket, there is no point adding this message to all responses, this is just noise if results are above the threshold. It could be added to e.g. service-info if it needs to be advertised more broadly.

@@ -2,6 +2,13 @@
from .exceptions import APIException, InvalidQuery


def no_results_censorship_message():
return (
"No results. Either none were found, or the query produced results numbering at "
Copy link
Member

Choose a reason for hiding this comment

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

this might get a bit weird with translation - maybe there should be two info messages in order or something... although translating anything with a dynamic count threshold is going to be weird. maybe we need a separate field in info with the configured threshold, and set some flag in info if we show a message regarding the threshold.

Copy link
Member Author

@gsfk gsfk Jan 10, 2024

Choose a reason for hiding this comment

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

Ah, I wasn't going to propose translating this at all, since:

  • it's the API json response, which is always English (even when searching in French in the front end)
  • if a user-facing translation is needed somewhere (eg in bento_public), it can be translated there rather than in the API
  • if bento public actually needs explicit mention of censorship, it needs to be somewhere other than hidden in an api response

Copy link
Member

Choose a reason for hiding this comment

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

yes I dont think the response itself needs to be translated, but having a dynamic message (i.e., a changing threshold) means it's hard to translate on the bento_public end - if the threshold is set in the response and a flag can be set to show a special second info box with the threshold included, it would be easier to translate on that end

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, there's only one item that changes, and it's in a fixed spot, so this is easily translated in two chunks.

At any rate, I don't expect bento_public to handle this message at all. This is intended only for people using beacon without a UI; if bento_public needs a message like this at all it should probably be mentioned in an "about" section or somewhere else.

@gsfk gsfk marked this pull request as ready for review January 11, 2024 14:04
@gsfk
Copy link
Member Author

gsfk commented Jan 22, 2024

Now split into two messages

@gsfk gsfk requested a review from davidlougheed January 22, 2024 14:56
Copy link
Member

@davidlougheed davidlougheed left a comment

Choose a reason for hiding this comment

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

lgtm

@gsfk gsfk merged commit 76dde0c into master Jan 23, 2024
1 check passed
@gsfk gsfk deleted the features/threshold-in-response branch January 23, 2024 12:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants