-
Notifications
You must be signed in to change notification settings - Fork 209
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
Workaround for legacy added games to have the ability to be deleted #6019
Conversation
… srietkerk-useradded
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.
lgtm, one possible suggestion just for cleanliness~
kiosk/src/Models/Kiosk.ts
Outdated
if (!game?.userAdded) { | ||
game.userAdded = true; | ||
} |
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.
if (!game?.userAdded) { | |
game.userAdded = true; | |
} | |
game.userAdded = true; |
should be fine to just mark all the games? fwiw this one caught my eye because the ?. on the previous line wouldn't actually protect us (as it would still get a null pointer exception on line 141 if game == null
, and similar on the one a few lines down (that won't get an npe, but would probably get some weird state if game == null
because this.games
would end up with a nullish entry in it.
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.
Should we just have a null check at the start of the inside of this for loop? It would certainly give some peace of mind just to make sure that if we have a nullish game, it won't be added to the list. I'm not sure if that's possible and it thus might be adding a branch for no reason, but it might just be a protection that we want to have.
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.
+1 to Joey's suggestion
When we first added the ability to delete user added games, we added a flag to the game entry so it would be easy to check if a game was user-added and thus add the "delete game" button under the game (seen here:
pxt-arcade/kiosk/src/Components/GameSlide.tsx
Line 40 in 53785bb
Because we've added the
userAdded=true
to every game that is added to kiosk now, this will only be a problem for kiosks that have older game uploads, but it's something that we should support. The fact that this "unable to delete" bug will only happen to older games is important, though, because on first load, kiosk fetches and parses through all of the user added games and pushes them to kiosk's game list. The kiosk game list is what we use to display all of the games together. Because we go through all of the added games, we can check to make sure that the local representation of those games have theuserAdded=true
flag.An alternative solution would be to, also on first load, go through all of the user added entries, update the
userAdded
flag and then update the localstorage object. However, I think this step adds more work than needed.Closes #6016