-
Notifications
You must be signed in to change notification settings - Fork 165
Fix Text Answer review redirect #2497
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
Fix Text Answer review redirect #2497
Conversation
niklasmohrin
left a comment
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 doesn't work yet. Firstly, when coming from the full view and editing an answer, I still get redirected to the quick view afterwards. Secondly, the question selection in the quick view also still shows the first unreviewed question, not the one from the fragment. Is it just broken on my computer?
Here is what I think we need to do: In quick-review-slider.ts we are calling this.startOver(StartOverWhere.Undecided);. I believe that we should move this out of QuickReviewSlider.attach and into the calling code (staff_evaluation_textanswers_quick.html) where we can then check if the page URL has a fragment for a specific textanswer - in that case we startOver with that. Otherwise, we do what we did before.
For the redirect into the correct view, I think we have to include the view we want to return to in the request to evaluation_textanswer_edit. We have this pattern already in the index view after logging in (check the end of the index function in evap/evaluation/views.py). Although here we don't need the full url, so maybe ?next-view=full or so would make for shorter / nicer urls.
|
I also think it would be awesome if we could get a quick frontend test going for this, otherwise someone might mess it up again in the future. After all, we didn't notice that the logic for the fragment is doing nothing after we introduced the quick view and redirected to it by default. To be fair, three years had passed between #530 and #1154 |
|
The redirect to the proper view (quick, full etc.) should work now. The view is passed to the edit view via url parameter inside |
|
@geromequa Do you want to continue working on this or should we hand it over to someone else? |
|
Current behavior: the page correctly jumps to the textanswer that was just edited. After editing a textanswer it is added to the back of the list of textanswers, unclear where this logic is implemented. Jumping to edited textanswer works without problems though. |
niklasmohrin
left a comment
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 is looking very promising, just some finishing touches are needed.
Most importantly, the quick view works, but removing the fragment= part breaks the functionality on the full view (I wrote a more detailed comment below). (there is no test for this, so it's not your fault)
Can you a) change the PR title to reflect the added functionality of the PR, and b) add a short frontend test for this behavior in the quick view?
I would also like to have a frontend test for the full view, but I don't know if there is a nice and robust way to do it since all the text is always shown in that view. Either way, you don't have to worry about that in this PR.
After editing a textanswer it is added to the back of the list of textanswers
The answers are just in whatever order the database gives them to us, I think they just appear in a random place. Do they always appear at the end for you? For me they also sometimes show up in the middle
niklasmohrin
left a comment
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.
Can you add (frontend) tests? Implementation looks good 👍
niklasmohrin
left a comment
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.
implementation looks good, tests need a little more work :)
evap/staff/tests/test_live.py
Outdated
| By.XPATH, "//div[@class='slider-item card-body active' and @data-layer='2']" | ||
| ) | ||
|
|
||
| self.assertEqual(str(answer[0].get_attribute("id")).split("-", 1)[1], str(textanswer1.pk)) |
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.
the [0] also seems like it could break arbitrarily depending on the order in which the answers were returned from the database; I think we can just check that the edited text is visible and the one from the other questions is invisible, take a look at existing tests that contain self.wait.until(visibility_of_element ...)
niklasmohrin
left a comment
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.
Very cool, thanks! I have some final remarks, but unless you have some questions or disagree, I don't need to review again
|
|
closes #1696