Add popover previews of (1) conversations with student, (2) response to the background questionnaire, and (3) points #109

etanttila · 2024-07-03T14:41:42Z

Description

What?

In the views of conversations, add popover previews of:

all conversations with student
student's response to the background questionnaire (enrollment exercise)
student's points (total points for (1) the course overall and by category, (2) the module, and (3) the chapter)

Popovers appear on hover and click. If clicked, they are dismissed when clicked anywhere.
Screencast from 2024-07-03 17:00:47.webm

The popovers work also on smaller screens and touch screens.
Screencast from 2024-07-03 17:18:19.webm

The background questionnaire popover displays only the questions that the student responded to and their responses (not all answer options) to make it more compact.

Screenshot of how the points popover appears:

(One of the categories is an empty string, which is why there isn't a label below the "A".)
The colors of the progress bar are the same/similar is in A+:

If there are some exercises in the chapter that have a points-to-pass value and the student hasn't passed some of them, the progress bar for the chapter is displayed red. Similarly for the module progress bar if the student doesn't have enough points to pass the module.
If the student has full points, the progress bar is displayed green.
If the student doesn't have full points but enough to pass (or there aren't limitations on points to pass), the progress bar is displayed yellow.

Why?

Allow teacher to gain a better understanding of the context (student's background, success on the course, previous conversations) when reading and responding to the student quickly and without having to leave the page.

How?

Uses bootstrap dismiss-on-next-click popovers.

The contents of a popover are computed the first time it's triggered, to reduce the amount of unnecessary rendering/computations and API calls. Therefore there can be some small delays, especially when displaying the points preview.

For the conversation preview, fetch contents of the conversation page filtering by student, taking just the conversations and removing all unnecessary content (e.g. buttons, feedback tags, etc.) to make it more compact, and insert this into the popover.

For the background questionnaire and points, implement new pages, which are used only to generate the contents for the popovers on demand. The contents are computed by these page's views and then inserted into the popovers.

For the background questionnaires, the enrollment exercise details as well as student responses are cached to reduce API calls and make calculations faster. A new object called BackgroundCache is created to help with setting and getting values to and from the cache.

Fixes #30 (the link part has been fixed before, this now adds the mouseover functionality)
Fixes #31 (doesn't add the direct link, which can be accessed by the student page, but the mouseover functionality, which was preferred by @jsorva)
Fixes #65

Testing

Remember to add or update unit tests for new features and changes.

How to test your changes in A-plus
How to test accessibility

What type of test did you run?

Tested the functionalities work with generated submissions, with different screen widths and also with the phone preview of the inspect tool.
Also tested hat with a multilingual course, the background questionnaire questions and answer options are displayed in the selected language.

Did you test the changes in

Chrome
Firefox
This pull request cannot be tested in the browser.

Think of what is affected by these changes and could become broken

Translation

Did you modify or add new strings in the user interface? (Read about how to create translation)

Programming style

Did you follow our style guides?
Did you use Python type hinting in all functions that you added or edited? (type hints for function parameters and return values)

Have you updated the README or other relevant documentation?

documents inside the doc directory.
README.md.
Aplus Manual.
Other documentation (mention below which documentation).

Tried to add explanatory comments to the code

I have not added documentation to Aplus manual, but indeed that should be added before the end of the summer.

Is it Done?

Reviewer has finished the code review
After the review, the developer has made changes accordingly
Customer/Teacher has accepted the implementation of the feature

Clean up your git commit history before submitting the pull request!

etanttila · 2024-07-04T07:41:48Z

This change requires a minor change to the A+ points API.

ihalaij1

Nice!

I hope I didn't miss any major programming errors, but here are my comments.

I noticed that the videos you linked seem to have empty space under the text of all the buttons, such as the Respond button, and the student tags. Compare the Respond button and the gray No response "tag" to the current one in this picture:

Do you see how in your video there is empty space, making the buttons and tags longer vertically?

I think something in the new CSS is making all the buttons on the page like that.

ihalaij1 · 2024-07-12T05:48:49Z

feedback/background_helpers.py

+    The dict has the question keys as keys. For checkboxes, the value is a
+    list of the answers. For other queston types, the value is the response.
+    The dict does not include questions that had no answers.


There's a typo in the sentence For other queston types...

ihalaij1 · 2024-07-12T05:54:57Z

feedback/cached.py

+    def get(self, key: str, course: Course):
+        full_key = '/'.join((self.prefix, self.get_suffix(key, course.id)))
+        return cache.get(full_key)


This is missing a return value type.

Good point. I thought that since this return value type would be Any, it could be left out. But on further thought, as it is either None or a picklable object (as per Django documentation), the type could probably be defined as an object

ihalaij1 · 2024-07-12T06:03:15Z

feedback/cached.py

+        full_key = '/'.join((
+            self.prefix,
+            self.get_suffix(student.id, course.id, 'response'))
+        )


I'd prefer the closing parenthesis of the tuple to be on the same line as the other closing parenthesis (with an extra comma too), like this:

full_key = '/'.join(( self.prefix, self.get_suffix(student.id, course.id, 'response'), ))

ihalaij1 · 2024-07-12T06:04:34Z

feedback/cached.py

+            full_key = '/'.join((
+                self.prefix,
+                self.get_suffix(student.id, course.id, 'response'))
+            )


I'd prefer the closing parenthesis of the tuple to be on the same line as the other closing parenthesis (with an extra comma too), like this:

full_key = '/'.join(( self.prefix, self.get_suffix(student.id, course.id, 'response'), ))

ihalaij1 · 2024-07-12T06:11:25Z

feedback/static/feedback.css

+.popover:has( .pos-right) {
+	left: auto !important;
+	right: 2rem !important;
+}


Is it absolutely necessary to use !important here? It is not a good practice to use it in CSS, unless there is no other way to make the styles work properly. If there is no other (relatively easy) way to make this work, add a comment explaining why we use !important here.

https://developer.mozilla.org/en-US/docs/Web/CSS/important#best_practices

Good point. The default location is dynamically defined inline, so for the conversation preview it needs to be defined with important, but I'm able to create workarounds for the other popovers.

ihalaij1 · 2024-07-12T06:11:44Z

feedback/static/feedback.css

+/* points display popovers */
+.popover:has( .points-display) {
+	left: auto !important;
+	right: 0 !important;
+	max-width: 400px;
+}
+.popover:has( .points-display) .arrow {
+	left: auto !important;
+	right: 1em !important;
+}


Is it absolutely necessary to use !important here? It is not a good practice to use it in CSS, unless there is no other way to make the styles work properly. If there is no other (relatively easy) way to make this work, add a comment explaining why we use !important here.

https://developer.mozilla.org/en-US/docs/Web/CSS/important#best_practices

ihalaij1 · 2024-07-12T06:20:33Z

feedback/views.py

+def update_context_for_feedbacks( # noqa
+        request: HttpRequest,
+        context: dict[str, Any],
+        course: Optional[Course] = None,
+        feedbacks: Optional[list[Feedback]] = None,
+        get_form: Optional[Callable[[Feedback], DynamicFeedbacForm]] = None,
+        post_url: bool = True
+        ) -> None:


Add a comma after post_url: bool = True

ihalaij1 · 2024-07-12T06:26:01Z

feedback/views.py

+    def student_has_no_bg_questionnaire(student: Student) -> bool:
+        resp = BackgroundCache.get_response(student, course) # pylint: disable=no-value-for-parameter
+        return (resp is not None) and (resp[0] is None)


Can this be changed to student_has_bg_questionnaire, so that we don't have to use a double negative later on line 571? Currently the line 571 is a bit silly to read:

'show_background': course_has_bg_questionnaire and not student_has_no_bg_questionnaire(conv.student),

The problem is that this doesn't check if the student has a background questionnaire (as if a student does, but it hasn't been computed yet, it wouldn't appear in the cache). This checks if it has been checked and the student didn't have a background questionnaire (so it doesn't need to be computed and a background questionnaire doesn't need to be displayed). This basically means that the button will show up the first time for a student even if they don't have a background questionnaire, and then if the button is triggered (and the API request is made and we discover the student doesn't have a response to the bg questionnaire, it's saved in the cache so we don't show the button falsely in the future. I guess this could be renamed to "student_might_have_bg_questionnare" or then the entire functionality could be changed so for all students if they don't have a response saved, we actually check if they have a response (and based on that show the button or not), which would slow down the loading somewhat.

If its difficult then it doesn't have to be changed. I was just thinking if it was easy, it would be a nice little change.

It's not difficult, it just slows down the initial loading time. (For example, if there are about 100 students whose responses haven't been loaded, when testing locally, loading the page with the feedbacks takes about 10 seconds instead of 2.) But otherwise it does make the UX probably a bit nicer (since the button doesn't appear if the student hasn't filled in the enrollment questionnaire) and indeed does make the code a bit more legible. So it would be a tradeoff. Hard to say which would be better.
I would suspect that decreasing the load time (and fetching the student responses on demand, even if it does mean sometimes in vain showing the "background" button if a student doesn't have a response) would be preferable.

Perhaps keeping the functionality as it is and changing the function name to "student_may_have_bg_questionnaire" (and having the respectively changing the return value) might be a good way of making the code a bit more legible.

etanttila · 2024-07-22T13:08:58Z

I noticed that the videos you linked seem to have empty space under the text of all the buttons, such as the Respond button, and the student tags. Compare the Respond button and the gray No response "tag" to the current one in this picture:

Do you see how in your video there is empty space, making the buttons and tags longer vertically?

I think something in the new CSS is making all the buttons on the page like that.

There indeed seems to be some empty space under the colortags, but that empty space appears also before these commits. (It seems to result from the default padding of the label class from bootstrap, and it hasn't been changed in this commit.) (Some additional space may also be from the video, due to low resolution)

The empty space below the buttons on the other hand might just be on the video and how that has recorded, it doesn't appear when running the code locally.

ihalaij1 · 2024-07-23T06:29:49Z

I was testing the style issues and came across an AttributeError in get_bg_questionnaires.
This happened when attempting to go directly to the URL in the screenshot as the root user (not accessing through the "Jutut" menu link in A+).

ihalaij1 · 2024-07-23T06:46:06Z

I noticed that the videos you linked seem to have empty space under the text of all the buttons, such as the Respond button, and the student tags. Compare the Respond button and the gray No response "tag" to the current one in this picture:
Do you see how in your video there is empty space, making the buttons and tags longer vertically?
I think something in the new CSS is making all the buttons on the page like that.

There indeed seems to be some empty space under the colortags, but that empty space appears also before these commits. (It seems to result from the default padding of the label class from bootstrap, and it hasn't been changed in this commit.) (Some additional space may also be from the video, due to low resolution)

The empty space below the buttons on the other hand might just be on the video and how that has recorded, it doesn't appear when running the code locally.

I tested running it now, it looks like there is no problem with the style now. Might have been just the video quality.

etanttila · 2024-07-23T12:09:53Z

I was testing the style issues and came across an AttributeError in get_bg_questionnaires. This happened when attempting to go directly to the URL in the screenshot as the root user (not accessing through the "Jutut" menu link in A+).

Good observation! I indeed haven't defined fallbacks for if the user doesn't have an aplustoken. I need to add checks in other functions as well that the client is defined, and decide how to deal with cases that there isn't a client. (E.g. should some parts of the page not be displayed, and/or should there be an error message saying that the user should log in via Aplus, or redirecting to an error page.)

etanttila · 2024-07-24T12:30:19Z

I now added checks that the client exists (e.g. has A+ api token) and added error messages if it doesn't. Basically these should appear only if the user logs in directly rather than via A+. If relevant information is cached, it's displayed to the user.

ihalaij1

I found some new issues with the previews that I think weren't there before.

For some reason, the background and points previews here are unnecessarily narrow:

Also, a minor style bug.
When hovering over the Points preview, the corner outlines of the button turn from being rounded into square:

This does not happen with the other two buttons:

etanttila · 2024-08-06T11:39:05Z

I found some new issues with the previews that I think weren't there before.

For some reason, the background and points previews here are unnecessarily narrow:

That's very strange, that does not happen for me (when testing with either Firefox or Chrome with Linux), also the buttons should be right-justified, unlike they seem to be for you. This is how it appears for me.

I wonder what causes that. I have no idea

Also, a minor style bug. When hovering over the Points preview, the corner outlines of the button turn from being rounded into square:

This does not happen with the other two buttons:

Good point, it seems to be due to the bootstrap button styling (having the popover in the same div messes up the styling), and similarly the buttons move around due to the changing margins when there are tooltips. These I can easily fix with styling.

ihalaij1

I tested again, and this time I flushed my browser cache.
Now the buttons have moved to the right side and the previews are no longer narrow.

Looks good!

ihalaij1

Actually, now the conversation preview is really far on the right side?
Is this a new issue?

etanttila · 2024-08-07T07:52:50Z

The conversation preview was placed to the right quite intentionally, (so it would cover the conversation as little as possible), but indeed it seems strange that it opens so far away from the button. We discussed with Jimmy that it might be better to move the two buttons ("Show all feedback for this student" and the conversation preview) to the right by the "Background" and "Points" buttons. This way the conversation preview popover could open on the right below the student panel just like the Background popover. This also means that the two buttons don't move around depending on the length of the student's name and the student tags can be immediately after the student name, which would be more intuitive. (The student conversation buttons weren't placed after the student tags because it would lead the location of the buttons to move around even more depending on the number and length of the student tags. Justifying the two buttons to the right resolves that problem.)
Of course, this does mean that we lose a bit of the consistency of having the "Show all feedback for this exercise" right behind the chapter/exercise name and the equivalent button for the student right behind the student name, but fortunately it still stays in the submitter heading / student info header, and improves the consistency of having the three popover/preview buttons near each other on the right.

So I'll make these changes as discussed with Jimmy.

etanttila · 2024-08-08T09:57:09Z

We also noticed when trying with the inspect view's mobile or responsive that when when near the bottom of the page, the page annoyingly scrolls up significantly so that the conversation that was just clicked isn't even visible anymore. Changing the container of the popover to 'body' reduced the issue significantly, even though it didn't fully fix the issue. (I wasn't able to come up with a better solution that would fix it entirely). Jimmy also pointed out that sometimes there may be several popovers open, which was a known issue, caused by manually opening the popovers when they are first triggered (due to the update of the popover content). These don't close unless triggered again, which could be very annoying. Therefore I added a function that manually closes the popover when anything else is clicked or focused after opening the popover (this occurs just once).

I also changed that the background questionnaire preview and and conversations-with-student preview are placed on top for the last conversation on the page if they don't fit below the button.

Fix a few typos and remove unnecessary "similar" strings from translation file.

Add a button that when hovered or clicked, a popover appears that displays all conversations with student. Fixes apluslms#30

Implement popover, which displays points that are relevant for the context of the given feedback. Adds a new view (with URL) that calculates the points for the student for the entire course (by categories), as well as the module and the chapter that the feedback was given from. This content is inserted into the popover. The popover displays the points in both numerical format as well as progress bars for the points by category, module and chapter. The total points are only displayed numerically. Requires A+ points API to provide max_points and max_points_by_difficulty for the course. Fixes apluslms#65

For each conversation, add a button that when hovered or clicked, a popover appears displaying the student's response to the enrollment questionnaire. For efficient usage when initially calculating the enrollment exercises, requires the A+ exercise api to contain the status in brief exercise serializer. Fixes apluslms#31

Change submitter heading (aka student info heading) layout in the feedback list view so that also buttons for viewing conversations with student are right-justified and near "Background" and "Points" buttons. This allows all three buttons that open popovers to be near each other and open the popovers on the right side of the viewport (overlaying feedback tags instead of conversation) without too much distance to buttons.

ihalaij1

Changes look good!

etanttila requested a review from ihalaij1 July 3, 2024 14:41

etanttila assigned ihalaij1 Jul 3, 2024

etanttila mentioned this pull request Jul 4, 2024

Add more info about max_points to points API apluslms/a-plus#1380

Merged

18 tasks

ihalaij1 requested changes Jul 12, 2024

View reviewed changes

etanttila mentioned this pull request Jul 22, 2024

Fix total conversation count when paginated #111

Merged

18 tasks

etanttila force-pushed the popups branch 2 times, most recently from 1d98c37 to 08677f1 Compare July 24, 2024 12:26

ihalaij1 requested changes Aug 6, 2024

View reviewed changes

ihalaij1 approved these changes Aug 7, 2024

View reviewed changes

ihalaij1 requested changes Aug 7, 2024

View reviewed changes

etanttila added 5 commits August 8, 2024 14:26

Fix minor errors

0c1a1f0

Fix a few typos and remove unnecessary "similar" strings from translation file.

Add popover preview of conversations with students

73ce4c2

Add a button that when hovered or clicked, a popover appears that displays all conversations with student. Fixes apluslms#30

ihalaij1 approved these changes Aug 8, 2024

View reviewed changes

etanttila force-pushed the popups branch from 39921d4 to 661f60c Compare August 8, 2024 11:44

etanttila merged commit f64aeee into apluslms:master Aug 8, 2024

etanttila deleted the popups branch August 8, 2024 11:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add popover previews of (1) conversations with student, (2) response to the background questionnaire, and (3) points #109

Add popover previews of (1) conversations with student, (2) response to the background questionnaire, and (3) points #109

etanttila commented Jul 3, 2024

etanttila commented Jul 4, 2024

ihalaij1 left a comment •

edited

Loading

ihalaij1 Jul 12, 2024

ihalaij1 Jul 12, 2024

etanttila Jul 22, 2024

ihalaij1 Jul 12, 2024

ihalaij1 Jul 12, 2024

ihalaij1 Jul 12, 2024

etanttila Jul 22, 2024

ihalaij1 Jul 12, 2024

ihalaij1 Jul 12, 2024

ihalaij1 Jul 12, 2024

etanttila Jul 12, 2024 •

edited

Loading

ihalaij1 Jul 12, 2024

etanttila Jul 22, 2024

etanttila Jul 22, 2024 •

edited

Loading

etanttila commented Jul 22, 2024 •

edited

Loading

ihalaij1 commented Jul 23, 2024 •

edited

Loading

ihalaij1 commented Jul 23, 2024

etanttila commented Jul 23, 2024

etanttila commented Jul 24, 2024

ihalaij1 left a comment •

edited

Loading

etanttila commented Aug 6, 2024

ihalaij1 left a comment

ihalaij1 left a comment

etanttila commented Aug 7, 2024 •

edited

Loading

etanttila commented Aug 8, 2024

ihalaij1 left a comment

Add popover previews of (1) conversations with student, (2) response to the background questionnaire, and (3) points #109

Add popover previews of (1) conversations with student, (2) response to the background questionnaire, and (3) points #109

Conversation

etanttila commented Jul 3, 2024

Description

Testing

Translation

Programming style

Have you updated the README or other relevant documentation?

Is it Done?

etanttila commented Jul 4, 2024

ihalaij1 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etanttila Jul 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etanttila Jul 22, 2024 • edited Loading

Choose a reason for hiding this comment

etanttila commented Jul 22, 2024 • edited Loading

ihalaij1 commented Jul 23, 2024 • edited Loading

ihalaij1 commented Jul 23, 2024

etanttila commented Jul 23, 2024

etanttila commented Jul 24, 2024

ihalaij1 left a comment • edited Loading

Choose a reason for hiding this comment

etanttila commented Aug 6, 2024

ihalaij1 left a comment

Choose a reason for hiding this comment

ihalaij1 left a comment

Choose a reason for hiding this comment

etanttila commented Aug 7, 2024 • edited Loading

etanttila commented Aug 8, 2024

ihalaij1 left a comment

Choose a reason for hiding this comment

ihalaij1 left a comment •

edited

Loading

etanttila Jul 12, 2024 •

edited

Loading

etanttila Jul 22, 2024 •

edited

Loading

etanttila commented Jul 22, 2024 •

edited

Loading

ihalaij1 commented Jul 23, 2024 •

edited

Loading

ihalaij1 left a comment •

edited

Loading

etanttila commented Aug 7, 2024 •

edited

Loading