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

Use standard forms instead of javascript submit for textanswer review #1705

Merged
merged 4 commits into from
Jul 4, 2022
Merged

Use standard forms instead of javascript submit for textanswer review #1705

merged 4 commits into from
Jul 4, 2022

Conversation

niklasmohrin
Copy link
Member

Closes #1698

janno42
janno42 reviewed Feb 7, 2022
View reviewed changes
Copy link
Member

@janno42 janno42 left a comment

Choose a reason for hiding this comment

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

new design idea: put all buttons in the same place on the page :)
image

evap/staff/templates/staff_evaluation_textanswers_section.html Outdated Show resolved Hide resolved
@niklasmohrin
Copy link
Member Author

Currently, there are warnings about having a content type header set with 204 responses (run python3 -We -W ignore::DeprecationWarning manage.py test to see the stacktraces), this will be exciting to fix 🙃

@niklasmohrin niklasmohrin force-pushed the standard-stuff-really branch 2 times, most recently from f6cc432 to 1cdb24c Compare March 14, 2022 21:33
@niklasmohrin niklasmohrin force-pushed the standard-stuff-really branch 4 times, most recently from 8f34059 to 5da2126 Compare March 28, 2022 21:43
@niklasmohrin niklasmohrin marked this pull request as ready for review March 28, 2022 21:47
richardebeling
richardebeling reviewed Apr 4, 2022
View reviewed changes
Copy link
Member

@richardebeling richardebeling left a comment

Choose a reason for hiding this comment

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

Code looks mostly good. Will have to test the UI manually. Some of the JS was hard to understand -- maybe we want to do something about it?

evap/staff/templates/quick-review.js Show resolved Hide resolved
evap/staff/templates/staff_evaluation_textanswers_full.html Outdated Show resolved Hide resolved
janno42
janno42 requested changes Apr 11, 2022
View reviewed changes
Copy link
Member

@janno42 janno42 left a comment

Choose a reason for hiding this comment

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

when selecting "yes" as publishing option for a textanswer, reloading the page and moving back to that answer in quick review, the "yes" button is not selected. it works for "no" though :D

evap/staff/templates/staff_evaluation_textanswers_quick.html Show resolved Hide resolved
evap/staff/templates/staff_evaluation_textanswers_section.html Outdated Show resolved Hide resolved
@niklasmohrin niklasmohrin force-pushed the standard-stuff-really branch 2 times, most recently from 6c6aef2 to 728a6b5 Compare April 11, 2022 20:26
@richardebeling
Copy link
Member

Firefox says no :(

Uncaught TypeError: btn is null
    updateButtonActive http://localhost:8000/staff/semester/21/evaluation/1600/textanswers:1082
    updateButtons http://localhost:8000/staff/semester/21/evaluation/1600/textanswers:1056
    startOver http://localhost:8000/staff/semester/21/evaluation/1600/textanswers:1031
    <anonymous> http://localhost:8000/staff/semester/21/evaluation/1600/textanswers:868
    jQuery 8
textanswers:1082:13
    updateButtonActive http://localhost:8000/staff/semester/21/evaluation/1600/textanswers:1082
    updateButtons http://localhost:8000/staff/semester/21/evaluation/1600/textanswers:1056
    startOver http://localhost:8000/staff/semester/21/evaluation/1600/textanswers:1031
    <anonymous> http://localhost:8000/staff/semester/21/evaluation/1600/textanswers:868
    jQuery 8

@niklasmohrin niklasmohrin force-pushed the standard-stuff-really branch from 728a6b5 to 1b68037 Compare May 9, 2022 17:36
@niklasmohrin niklasmohrin mentioned this pull request May 9, 2022
Open
richardebeling
richardebeling reviewed May 9, 2022
View reviewed changes
Copy link
Member

@richardebeling richardebeling left a comment

Choose a reason for hiding this comment

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

(Python) code still looks good. Still feel unable to properly review the html/js stuff.

@fidoriel fidoriel mentioned this pull request May 11, 2022
Closed
@richardebeling
Copy link
Member

richardebeling commented May 17, 2022 •
edited
Loading

Since you are editing these files: The message "If you should edit an answer [...]" in staff_evaluation_textanswer.html, around L39, should be changed to "If you edit an answer [...]". If you want, you can add this here, otherwise, I'll open an issue.

janno42
janno42 previously approved these changes May 23, 2022
View reviewed changes
Copy link
Member

@janno42 janno42 left a comment

Choose a reason for hiding this comment

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

✔️ Meets requirements
✔️ UI functionality checked

@janno42 janno42 dismissed their stale review May 29, 2022 17:59

pr in unfinished state

@niklasmohrin niklasmohrin requested a review from janno42 June 13, 2022 18:26
richardebeling
richardebeling approved these changes Jun 13, 2022
View reviewed changes
Copy link
Member

@richardebeling richardebeling left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Code looks good, tested manually for a bit, worked fine 👍

janno42
janno42 requested changes Jun 20, 2022
View reviewed changes
Copy link
Member

@janno42 janno42 left a comment

Choose a reason for hiding this comment

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

  • The text of the third view button in the top right at the text answer review page should be changed into "Undecided"
  • In quick view, the edit button sometimes links to the wrong text answer and the actions are also not always performed on the correct text answer

@niklasmohrin
Copy link
Member Author

I think this PR is getting really big, I would do the renaming in a follow-up

@Kakadus Kakadus mentioned this pull request Jun 27, 2022
Closed
janno42
janno42 approved these changes Jul 4, 2022
View reviewed changes
Copy link
Member

@janno42 janno42 left a comment

Choose a reason for hiding this comment

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

✔️ Meets requirements
✔️ UI functionality checked

@niklasmohrin
Add HttpResponseNoContent response class
b753a9c 
It is analogous to the builtin `HttpResponseNotModified` which cannot
have a content type header either. This fixes the webtest lint

```
WSGIWarning: Content-Type header found in a 204 response, which should not return content.
```
@niklasmohrin
Rename FileResponse -> AttachmentResponse and add docstring
3f25d52
@niklasmohrin
Change state transition test helper and test for new behavior
f017982 
Of course, I did this strictly before implementing the feature, just
like I have always been told to do.
@niklasmohrin
Use forms instead of ajax for textanswer review
c54da99 
The forms submit to the same endpoint which now returns http 204 on
success instead of 200. This instructs the browser not to reload the
page, which is equivalent to what it did before, but with fewer custom
behavior.

In the "full" view, there are now four buttons instead of three.
They correspond to the four states that a textanswer can be in.
Before, the fourth state "undecided" / "unreviewed" was displayed by all
buttons being inactive.

Additionally, I renamed the action "hide" to "delete" and the request
parameter "id" to "answer_id".
@niklasmohrin niklasmohrin force-pushed the standard-stuff-really branch from 612bcf0 to c54da99 Compare July 4, 2022 17:47
@niklasmohrin niklasmohrin merged commit e083f3e into e-valuation:main Jul 4, 2022
@niklasmohrin niklasmohrin deleted the standard-stuff-really branch July 4, 2022 17:59
@niklasmohrin niklasmohrin mentioned this pull request Jul 4, 2022
Merged
@niklasmohrin niklasmohrin mentioned this pull request Jul 18, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@janno42 janno42 janno42 approved these changes

@richardebeling richardebeling richardebeling approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use forms for staff/evaluations/textanswers_* instead of Ajax
3 participants
@niklasmohrin @richardebeling @janno42