Allow students to download a copy of their answer table. #43
Conversation
|
@e-kolpakov Since you didn't review this earlier, I went ahead and appended the sharing feature to it. Ready for your review. |
problem_builder/table.py
Outdated
There was a problem hiding this comment.
@Kelketek shared_with and view_options could be sets - in such case it wouldn't be necessary to check if it's already there - just add to a set.
|
@Kelketek aside from two improvement suggestions, there are a couple of possible bugs:
Also, it was a bit unexpected that "Share" dialog didn't close when clicked outside of it. Maybe I'm got a bit too used to closing popups by clicking anywhere :) |
Kelketek
force-pushed
the
download-table
branch
2 times, most recently
from
c156c8b to
21e046a
Compare
|
@Kelketek I'm still looking at this but here's some early feedback:
|
|
@Kelketek This may be a minor issue, but: In the LMS, there's some CSS rule in Problem Builder that makes the columns of the table even in width (I can't remember what/where at the moment unfortunately): But in the downloaded report, the CSS is different and the widths are uneven: Also nit: There should be a bit of space between "Date: blah" and the table in the downloaded report. |
|
@Kelketek The Share UI has zero instructions and is super confusing: Is this going to post to Twitter? What is this for? What am I supposed to type into the box? Do I click "+" or "Share" once I've entered something? I think at the least it needs to explain what the sharing does and have a placeholder attribute on the text input saying "Enter a username" or something. |
|
Ooh, this is nice. It has words and everything! |
problem_builder/table.py
Outdated
There was a problem hiding this comment.
This might need to be changed :-/
Scope.user_state_summary fields have no locking so if two students make changes at the same time, the second one will overwrite the first one. It also could easily lead to these two fields being out of sync, if a different student "wins" for each write.
Quote from Dave when I proposed something similar: "I would strongly discourage you from using Scope.user_state_summary for this purpose. As you point out, that scope was not meant to be used for items with high concurrency needs, and write conflicts will happen."
There was a problem hiding this comment.
Since we're in a time crunch I would suggest storing this data in a Django table instead. And get rid of the username->anonymous ID map table; you can include that data in the new shares table if necessary.
|
✨ 👍 ✨ (when the tests pass) |
Kelketek
force-pushed
the
download-table
branch
2 times, most recently
from
33c318e to
767d86f
Compare
Allow students to download a copy of their answer table.
|
@Kelketek Thanks for getting this merged in time! Hope you have good holidays : ) @bradenmacdonald I read quickly on IRC this morning that you had concerns about the UX on this story. Is it still the case or were they addressed as part of the review? If there are still concerns, can you let me know what you think needs to be improved? |
|
@antoviaque I did but @Kelketek fixed my main concern, which was that there was no explanation or instructions in the UI. That's now been addressed during the review. I do have one remaining minor concern which is that the "+" button behaviour for sharing with multiple people is a bit strange (when you click the "+" button, you get another one, so there are now two "+" buttons, and you can get many "+" buttons appearing even though only one is ever needed). But that's very minor. |
|
@bradenmacdonald OK, great, thanks. And agreed about the '+' button, it's true that it would have been better to only keep one, next to the last input field. But yes, it's minor. |
No description provided.