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

Initialize setting review / proposal stage page #782

Draft
wants to merge 24 commits into
base: master
Choose a base branch
from

Conversation

alice6373
Copy link
Contributor

@alice6373 alice6373 commented May 21, 2020


name: Develop a configuration page for setting the review / proposal stage
about: #727
title: Initialize setting review / proposal stage page
labels: review-stages
assignees: ''


Description

Initialize setting review / proposal stage page
image

Steps to Test This Pull Request

Steps to reproduce the behavior:

  1. Go to 'accounts/login/'
  2. Login admin account
  3. Click on 'Change Review or 更改審查設定'
  4. Click on any button

Expected behavior

  • Only admin can see this page.
    (The action corresponding to the button has not been completed.)
  • Quick buttons corresponding to Proposal Review System Documentation)
  • Submit Button for configuration the review / proposal stage setting
    (Use dj-registry package)
  • Confirm whether the value of each attribute in Entries (DJANGO REGISTRY) has changed
  • Setting load current setting in render

More Information

Page Screenshots
setting the review / proposal stage

select timezone

Django Entries

@codecov-commenter
Copy link

codecov-commenter commented May 21, 2020

Codecov Report

Merging #782 into master will decrease coverage by 0.38%.
The diff coverage is 25.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #782      +/-   ##
==========================================
- Coverage   75.05%   74.67%   -0.39%     
==========================================
  Files          80       80              
  Lines        3083     3107      +24     
==========================================
+ Hits         2314     2320       +6     
- Misses        769      787      +18     
Impacted Files Coverage Δ
src/users/urls.py 100.00% <ø> (ø)
src/users/views.py 75.00% <19.04%> (-10.59%) ⬇️
src/proposals/templatetags/proposals.py 90.00% <66.66%> (-10.00%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update acec059...db4fea7. Read the comment docs.


.input-customized-size input{
width: 16.2em;
}
Copy link
Member

Choose a reason for hiding this comment

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

nitpicking: no new line in the end of a file

Copy link
Member

@tai271828 tai271828 left a comment

Choose a reason for hiding this comment

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

Overall looks good to me. Thank you for the effort and contribution. I can't wait for this beautiful UI and feature! Some nitpicking comments inline for your reference.

@tai271828 tai271828 requested a review from uranusjr May 30, 2020 15:02
@tai271828
Copy link
Member

@alice6373 for now @uranusjr will help you complete this PR. Please feel free to discuss with him if you have any thought about this PR : )

src/users/models.py Outdated Show resolved Hide resolved
src/users/views.py Outdated Show resolved Hide resolved
@alice6373 alice6373 force-pushed the page-for-setting-review-stage branch from 9daa732 to 1497d90 Compare July 18, 2020 08:47
Copy link
Member

@yychen yychen left a comment

Choose a reason for hiding this comment

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

I just did a very quick test. It looks like when I first get into the setting page, it didn't read the current registry values.

proposals_creatable.checked = false;
proposals_editable.checked = false;
proposals_withdrawable.checked = false;
reviews_stage.value = "0";
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be 1 (?)


{% if user.is_superuser %}
<li class="{% if active == 'admin' %}active{% endif %}">
<a href="{% url 'review_change' %}">{% trans 'Change Review' %}</a>
Copy link
Member

Choose a reason for hiding this comment

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

Can we call it Review Stages? "Change Review" seems a little bit imprecise here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your advise. To rename, I push a new commit db4fea7. Review Stages is indeed more precise.

@codecov-io
Copy link

codecov-io commented Oct 12, 2020

Codecov Report

Merging #782 (c4ef31e) into master (3e4e35a) will decrease coverage by 0.79%.
The diff coverage is 24.48%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #782      +/-   ##
==========================================
- Coverage   75.05%   74.26%   -0.80%     
==========================================
  Files          80       80              
  Lines        3083     3132      +49     
==========================================
+ Hits         2314     2326      +12     
- Misses        769      806      +37     
Impacted Files Coverage Δ
src/users/urls.py 100.00% <ø> (ø)
src/users/views.py 85.58% <ø> (ø)
src/reviews/views.py 30.12% <20.00%> (-3.77%) ⬇️
src/proposals/templatetags/proposals.py 90.00% <66.66%> (-10.00%) ⬇️
src/reviews/urls.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3e4e35a...c4ef31e. Read the comment docs.

src/users/views.py Outdated Show resolved Hide resolved
Copy link
Member

@yychen yychen left a comment

Choose a reason for hiding this comment

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

I know there are too many details in the front-end part. But these are required if you want to deliver a smooth user experience. I think we're almost there.

src/templates/default/reviews/review_stages.html Outdated Show resolved Hide resolved
Comment on lines 81 to 82
<input name="proposals.disable.after" type="datetime-local" step="1"
placeholder="yyyy-mm-dd hh:mm:ss" required="required">
Copy link
Member

Choose a reason for hiding this comment

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

I am having doubts about making this field "required" because I think we should be able to disable it by not providing any value?

And another thing is that the backend needs the format exactly as YYYY-mm-ddThh:mm:ss, you need to put the T in there, but the placeholder hints without the T. I personally think that YYYY-mm-dd hh:mm:ss should be more user-friendly, and you can consider accepting values without ss (treat missing ss as 0).

You can take a look at how django implements this here:
https://github.com/django/django/blob/master/django/forms/fields.py#L366

They take multiple possible formats and try one another, described in here https://docs.djangoproject.com/en/3.0/ref/settings/#datetime-input-formats

I think this is a little bit too much, but if you have time you can try this. The bottom line is at least make it more user friendly and the frontend hint needs to meet the backend validation criteria.

Copy link
Contributor Author

@alice6373 alice6373 Dec 20, 2020

Choose a reason for hiding this comment

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

Consider accepting values without ss -> 9ef909e

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But on the other hand, what does it mean that we should be able to disable it by not providing any value ? Is it for users to change a value individually?

Copy link
Member

Choose a reason for hiding this comment

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

What I meant is that, proposals.disabled.after should take effect if this key / value is set. If no value is set for this key, like, an empty value, then it will never be disabled after any time.

fmt = '%Y-%m-%d %H:%M:%S%z'
tz_selectd = pytz.timezone(request.POST['review_timezone'])
date_time_obj = datetime.datetime.strptime(
request.POST['proposals.disable.after'], '%Y-%m-%dT%H:%M:%S')
Copy link
Member

Choose a reason for hiding this comment

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

I suppose this is the mismatched part. Acutally it will raise an exception without telling the user what went wrong. A better to do this is at least tell the user about the error format so that they know what happened.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the Google browser, datetime-local allows users to click on the date with a graphical button, but in Safari browsers it is indeed to use text input, so there is the above problem.

Refer to your suggestion and use validation error. Its changes such as d579e69. When inputting a date format that cannot be parsed, the following prompt screen will be generated.

image

Copy link
Member

Choose a reason for hiding this comment

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

Yep, so here comes the validation part. If the value provided is invalid, it should return back to the frontend with some error messages so that the user know what should be fixed (usually some error message in red near the field, telling what's wrong and what should be expected)

Copy link
Contributor Author

@alice6373 alice6373 Jan 16, 2021

Choose a reason for hiding this comment

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

Thank Tom. @mattwang44 also advised me used message.error instead of debug mode to remind users.
Done !
image

Comment on lines 110 to 114
{% block extra_js %}
{% compress js %}
<script src="{% static 'js/reviews/review_stages.js' %}"></script>
{% endcompress %}
{% endblock extra_js %}
Copy link
Member

Choose a reason for hiding this comment

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

I think you should read current values from the database and show current settings on page load.

Something like

<script src="{% static 'js/reviews/review_stages.js' %}"></script>
<script>
proposals_creatable.checked = {% if proposals_creatable %}true{% else %}false;
proposals_editable.checked = {% if proposals_editable %}true{% else %}false;
proposals_withdrawable.checked = {% if proposals_withdrawable %}true{% else %}false;
reviews_stage.value = {{ reviews_stage }};
reviews_visible_to_submitters.checked = {% if reviews_visible_to_submitters %}true{% else %}false;
</script>

The above is just a sample code, very ugly, but just a simple demonstration of how django template variables and javscript can be integrated together.

Copy link
Contributor Author

@alice6373 alice6373 Dec 27, 2020

Choose a reason for hiding this comment

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

There are currently two ways to display the current settings. The first is that your suggestions are automatically loaded into the form that will be submitted (500b9db). The second is to list the current settings in another list (2fb312d).

But the web page is a bit crowded.
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe the two functions of the current review stage status and the set review stage can be presented in different pages ?Do you have more ideas ?

Copy link
Member

Choose a reason for hiding this comment

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

uh... what I mean is that the boolean switches should represent the current set values. So that when the admin sees the page, current settings are clear. And the admin can do some minor tuning without "not knowning" what current values look like.

我想用中文可能比較好表達。原本是不會載入目前的設定的,所以每次進來這一頁都是 false、或者是空的值。這樣其實很難一目瞭然目前到底是哪一個階段,或者是說目前的設定到底是長怎樣。期待的樣子是我進來的時候,所有的值就是目前的設定。舉個很爛的例子,假設目前是 editable、withdrawable 是開的,然後突然要很臨時的開放 creatable 一小段時間,我就只要把 creatable 打開,然後時間過了我再把它關起來。我就不需要再一個一個確認其他某一個的值是什麼,然後小心翼翼的調整怕弄錯,然後改完又要再小心翼翼的改回去。

@mattwang44
Copy link
Member

@mattwang44 mattwang44 mentioned this pull request Dec 29, 2020
6 tasks
Copy link
Member

@yychen yychen left a comment

Choose a reason for hiding this comment

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

I think most of it seems good. Just, still, the proposals.disabled.after settings, I think it should not be required.

src/templates/default/reviews/review_stages.html Outdated Show resolved Hide resolved
Copy link
Member

@yychen yychen left a comment

Choose a reason for hiding this comment

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

Actually, I still don't know why you don't want to load the proposals.disable.after date in the frontend like all other boolean switches.

In src/templates/default/review/review_stages.html, merely add some code we can also provide the saved datetime and put it there.

I think the same idea also holds. If proposals.disable.after is set, and an admin wants to switch the one of the switches but forget to set the time explicitly, this setting will be lost, which is also a little bit of dangerous for me.

There's also something different, did you remove the datetime picker javascript widget? I did not see it this time.
圖片

src/reviews/views.py Show resolved Hide resolved
request, 'reviews/review_stages.html', {
'timezones': pytz.common_timezones,
'review_stages_list': review_stages_list,
'current_review_stages_setting': current_review_stages_setting,
Copy link
Member

Choose a reason for hiding this comment

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

If you go to this path, current_review_stages_setting will be empty, and all values are lost. If the user entered an invalid value for proposals.disable.after, and then corrected it, and press save, all previous states will be lost, which is dangerous.

I think this is some kind of duplicate code of the normal render() for this view. maybe you should make them the same code.

@codecov-commenter
Copy link

codecov-commenter commented Apr 18, 2021

Codecov Report

Merging #782 (c4ef31e) into master (3e4e35a) will decrease coverage by 0.36%.
The diff coverage is 24.48%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #782      +/-   ##
==========================================
- Coverage   75.05%   74.69%   -0.37%     
==========================================
  Files          80       84       +4     
  Lines        3083     3367     +284     
==========================================
+ Hits         2314     2515     +201     
- Misses        769      852      +83     
Impacted Files Coverage Δ
src/users/urls.py 100.00% <ø> (ø)
src/users/views.py 85.82% <ø> (+0.24%) ⬆️
src/reviews/views.py 30.12% <20.00%> (-3.77%) ⬇️
src/proposals/templatetags/proposals.py 90.00% <66.66%> (-10.00%) ⬇️
src/reviews/urls.py 100.00% <100.00%> (ø)
src/sponsors/views.py 41.02% <0.00%> (-36.76%) ⬇️
src/core/templatetags/pycontw_tools.py 92.30% <0.00%> (-7.70%) ⬇️
src/core/context_processors.py 91.42% <0.00%> (-4.73%) ⬇️
src/core/models.py 92.75% <0.00%> (-1.89%) ⬇️
src/proposals/admin.py 100.00% <0.00%> (ø)
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3e4e35a...c4ef31e. Read the comment docs.

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.

8 participants