-
Notifications
You must be signed in to change notification settings - Fork 38
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
#3414: Configured setting for displaying total review score on reviews page #3415
base: main
Are you sure you want to change the base?
Conversation
65974ad
to
431833b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. As it won't affect the current visibility so there shouldn't be any issue but I was thinking to use these kinds of settings as wagtail settings so that these can be tested and changeable easily by any organization.
What do you think @gmurtaza00 @frjo @theskumar?
hypha/settings/base.py
Outdated
@@ -478,3 +478,5 @@ | |||
debug=SENTRY_DEBUG, | |||
integrations=[DjangoIntegration()] | |||
) | |||
|
|||
DISPLAY_TOTAL_REVIEW_SCORE = env.bool('DISPLAY_TOTAL_REVIEW_SCORE', default = False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be moved up to the Hypa settings section. We want all Hypha settings together in one place.
We want to move a lot of settings in to a Hypha settings section in the Web app. I think we need to decide how that is going to work overall first. |
|
||
If True sum of scores will be displayed otherwise average of scores will be displayed | ||
|
||
DISPLAY_TOTAL_REVIEW_SCORE = env.bool('DISPLAY_TOTAL_REVIEW_SCORE', default = False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make it an option instead of boolean. Current boolean make a lot of implicit assumption. It's a either/or situation but not reflected well with the name of the variable.
Make this setting a take a value, and have it also accept empty for use-cases where no final summary is desired by any setup.
Suggestion:
from typing import Literal
REVIEWS_FINAL_SCORE_METHOD: Literal['sum', 'avg', ''] = env.str('REVIEWS_SUMMARY_METHOD', default='sum')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@theskumar For cases like empty string, should we return NA from calculate_score function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
None
or NA
seems fine with me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we are introducing a configuration let's try make it more maintainable. Added by recommendation inline cc: @frjo
Configured a setting for displaying total review score instead of average score. Each organization can customize it accordingly.
Setting to consider:
DISPLAY_TOTAL_REVIEW_SCORE
Closes #3414