My Saved Decks#2117
Conversation
This allows us to set an assignment from anywhere a card is displayed, rather than requiring a redirect to the set assignments page
I have refactored the Ada "Remove board" button for consistency, but I'd really like to remove it in favour of the Sci approach. Decision on this pending from Ada team. Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2117 +/- ##
==========================================
+ Coverage 43.76% 43.81% +0.05%
==========================================
Files 591 593 +2
Lines 24944 24970 +26
Branches 7389 8295 +906
==========================================
+ Hits 10916 10940 +24
+ Misses 13978 13972 -6
- Partials 50 58 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
[VRT] Update baselines for feature/my-saved-decks
sjd210
left a comment
sjd210
left a comment
There was a problem hiding this comment.
There's a lot here and most of it still seems to be working as I expect, but there are a few things to look at/discuss.
Love the spinning star animation btw :)
| <ShareLink linkUrl={boardLink} reducedWidthLink clickAwayClose size="sm" buttonProps={{color: "keyline"}} /> | ||
| </div>} | ||
| {isTutorOrAbove(user) && <Button className="flex-grow-1" color="keyline" onClick={(e) => {e.preventDefault(); openAssignModal?.();}}> | ||
| {isSetAssignments ? "Assign / Unassign" : "Assign"} |
There was a problem hiding this comment.
This is missing the previous logic of also only using "Assign" when there are 0 groups that the gameboard is currently set to. It feels clearer to me that we keep this (and also it still has this behaviour on the table view, so even if we did want to change it, this is inconsistent with that)
There was a problem hiding this comment.
Ah, sorry, there's additional context here – Set Assignments (soon to be Manage Assignments) will only show assigned decks, so there'll never be a case where there are 0 groups assigned and isSetAssignments is true. You'll also need to go to Manage Assignments to _un_set an assignment, so Unassign won't be an option outside of it.
I'll update table view to match!
| const confirmMessage = board.ownerUserId === user.id && !board.tags?.includes("ISAAC_BOARD") | ||
| ? `Are you sure you want to unlink your board '${board.title}' from your account? You'll only be able to find it again if you've set it as an assignment.` | ||
| : `Are you sure you want to unlink '${board.title}' from your account?`; | ||
| if (confirm(confirmMessage)) { |
There was a problem hiding this comment.
This has strange interactions with the existing deck deletion code at https://github.com/isaacphysics/isaac-react-app/blob/main/src/app/state/slices/api/gameboards.ts#L162.
- An admin may get two popups in a row when unlinking a deck that has been assigned - it feels like we can combine these into one.
- A non-admin gets an error after they have confirmed unlinking in this same scenario. If we want to maintain requiring a teacher to be linked to a deck while it is assigned, we shouldn't have that confirmation box appear in the scenario and should just fail early.
- That said, maybe we shouldn't be enforcing it in this new system anyway? It was already inconsistent for groups with multiple managers (since only one manager would need to be linked to and set the assignment for it to apply to that whole group), and since saving is now a much more separate/manually controlled action, it seems more consistent to give them full control over saving.
There was a problem hiding this comment.
Agree with your last point; to me, the spirit of these changes are to make saving a completely separate interaction to creating, assigning, etc. You should be free to unsave a deck if you're done with it, regardless of whether it is assigned, so long as it is still accessible from somewhere. Right now, this last point is not met, since I have not yet done the changes to Set Assignments; but when that shows all assigned decks, this should work as expected.
I've removed the whole chunk of code around stopping this; it's obviously a little weird right now with not being able to access assigned ones, but this should be resolved soon.
| const user = useAppSelector(selectors.user.loggedInOrNull); | ||
|
|
||
| const [justLinked, setJustLinked] = useState(false); | ||
| const isLinked = useMemo(() => board.savedToCurrentUser || justLinked, [board, justLinked]); |
There was a problem hiding this comment.
This seems to be an API issue, but I'll leave a comment here while it's on my mind and it can be worked on later like the other known issues. This board.savedToCurrentUser is not behaving as it should for gameboard pages themselves. The board appears as saved if and only if the user has attempted any questions on it (as would've previously created a link).
Using the button does successfully save/unsave boards if in the right state to do so, but that state does not reflect the actual saved-ness of the board and whether it shows up in "My saved decks".
There was a problem hiding this comment.
Ah, thanks for spotting this. There's a card for savedToCurrentUser not working for My Assignments and book pages, but this seems to be a third case.
4 participants
Reworks My Question Decks so that only decks that are created or manually saved get saved to that page; no longer save decks that were simply attempted. Adds a save button (I used a star because this (will) appear on a different page to bookmarks – please disagree if you don't like it) to all decks to achieve this. Also renames the page to My Saved Decks to better reflect these changes.
Anyone should be able to save and view their saved decks, but teachers should also have the ability to set decks from anywhere without having to redirect to the Set Assignments page. To better enable this, a refactor of GameboardCards was undertaken.
Note that both Isaac and Ada have both requested the changes from automatic saving on attempt to manual saving only, but that the pages in which these are displayed are different; Ada does not have a My Saved Decks page or equivalent.
This is the first part of the wider changes to managing assignments; this does not rework Set Assignments in any meaningful way (though the cards do look slightly different).
Known issues:
savedToCurrentUser, so they do not appear saved when loading the page;