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

Add Review Stages Setting #1204

Open
wants to merge 31 commits into
base: master
Choose a base branch
from

Conversation

tomatoprinx
Copy link
Contributor

@tomatoprinx tomatoprinx commented Jul 13, 2024

Types of changes

  • Bugfix
  • New feature
  • Refactoring
  • Breaking change (any change that would cause existing functionality to not work as expected)
  • Documentation Update
  • Other (please describe)

Description

Steps to Test This Pull Request

Steps to reproduce the behavior:

  1. Login with admin account
  2. Go to 'Review Stages' page
  3. Click hotkeys in the 'Set Review Stage' section
  4. Submit

Expected behavior

Related Issue

#782

More Information

reviews_stage_setting

Copy link

codecov bot commented Jul 21, 2024

Codecov Report

Attention: Patch coverage is 98.11321% with 1 line in your changes missing coverage. Please review.

Project coverage is 74.44%. Comparing base (0e6ad43) to head (ee689cc).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
src/reviews/views.py 97.91% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1204      +/-   ##
==========================================
+ Coverage   74.03%   74.44%   +0.40%     
==========================================
  Files          81       81              
  Lines        3062     3115      +53     
==========================================
+ Hits         2267     2319      +52     
- Misses        795      796       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@josix josix marked this pull request as ready for review September 17, 2024 10:06
@tomatoprinx tomatoprinx requested a review from josix September 18, 2024 15:34
@tomatoprinx tomatoprinx requested a review from mattwang44 January 6, 2025 16:17
Copy link
Member

Choose a reason for hiding this comment

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

please fix the translation entries with fuzzy marks

Comment on lines +400 to +403
datetime_input_format += ['%Y-%m-%dT%H:%M:%S', '%Y-%m-%dT%H:%M']
value = value.strip()
# Try to strptime against each input format.
for format in datetime_input_format:
Copy link
Member

Choose a reason for hiding this comment

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

it's better not to mutate the input arguments

Suggested change
datetime_input_format += ['%Y-%m-%dT%H:%M:%S', '%Y-%m-%dT%H:%M']
value = value.strip()
# Try to strptime against each input format.
for format in datetime_input_format:
value = value.strip()
# Try to strptime against each valid format
valid_formats = datetime_input_format + ['%Y-%m-%dT%H:%M:%S', '%Y-%m-%dT%H:%M']
for format in valid_formats:

current_review_stages_setting[tag] = value


def date_preprocess(datetime_input_format, value):
Copy link
Member

Choose a reason for hiding this comment

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

give the arg a plural name if it's a list

Suggested change
def date_preprocess(datetime_input_format, value):
def date_preprocess(datetime_input_formats: list[str], value: str) -> datetime.datetime | None:

Comment on lines +29 to +30
function Call_for_Proposals() {
proposals_creatable.checked = true;
Copy link
Member

Choose a reason for hiding this comment

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

nit: we typically use camel case for class/variable names in javascript

Suggested change
function Call_for_Proposals() {
proposals_creatable.checked = true;
function CallForProposals() {
proposalsCreatable.checked = true;

Comment on lines 17 to +18
padding: $panel-body-padding;
color: black;
Copy link
Member

Choose a reason for hiding this comment

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

ensure same indent style

Suggested change
padding: $panel-body-padding;
color: black;
padding: $panel-body-padding;
color: black;

@@ -327,3 +332,77 @@ def get_success_url(self):
if query_string:
return url + '?' + query_string
return url


def review_stages(request):
Copy link
Member

Choose a reason for hiding this comment

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

nit: consider rewriting it into a class-based view, as it seems to be one of our conventions

value = reg.get(key, '')
update_current_review_stages_setting(tag, value, current_review_stages_setting)

if request.method == 'POST':
Copy link
Member

Choose a reason for hiding this comment

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

what are the allowed methods of this api? can i call with PUT, PATCH, GET?

</div>
</li>
<li class="panel-title list-group-item">
timezone
Copy link
Member

@mattwang44 mattwang44 Jan 11, 2025

Choose a reason for hiding this comment

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

What's the purpose of allowing updating the timezone?
Aren't we switching the stages based AoE (Anywhere on Earth) time? The README of the review stages does not mention timezone too.

Comment on lines +393 to +395
if "." in tag:
tag = tag.replace(".", "_")
current_review_stages_setting[tag] = value
Copy link
Member

Choose a reason for hiding this comment

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

to relieve the maintainers/developers from the burden to remember the conversion logic, can we align the naming of the reg keys and context pass to the template?

date_time_obj = date_preprocess(DATETIME_INPUT_FORMATS, request.POST[tag])
if date_time_obj is None:
messages.error(request, 'Please input valid date format : " + "%Y-%m-%dT%H:%M')
value = None
Copy link
Member

Choose a reason for hiding this comment

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

why save None on error? maybe we can raise exception and respond with 400 instead?

@mattwang44
Copy link
Member

mattwang44 commented Jan 11, 2025

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.

3 participants