Skip to content
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

Restore leaderboards #250

Merged
merged 11 commits into from
Oct 23, 2024
Merged

Restore leaderboards #250

merged 11 commits into from
Oct 23, 2024

Conversation

zysim
Copy link
Collaborator

@zysim zysim commented Oct 14, 2024

Closes: #240

I do not know how to write these dang tests. I now do know how to write these dang tests.

@zysim zysim self-assigned this Oct 14, 2024
@zysim zysim requested a review from a team as a code owner October 14, 2024 15:30
@Eein
Copy link
Contributor

Eein commented Oct 16, 2024

If leaderboard change is successful, frontend will probably want the leaderboard returned with the changes instead of a NoContent;

They will likely have to update a local cache/store - so this is better than making another request.

@zysim zysim force-pushed the restoreLeaderboards branch from 2ea53b0 to 3041388 Compare October 16, 2024 13:46
@zysim zysim requested a review from Eein October 16, 2024 13:46
Copy link
Contributor

@TheTedder TheTedder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we forgot to cover the case where the board can't be restored because its slug has been reclaimed by another board. Shall we simply have it return a 409 and possibly the conflicting board or at least the ID of the conflicting board?

LeaderboardBackend.Test/Leaderboards.cs Outdated Show resolved Hide resolved
LeaderboardBackend.Test/Leaderboards.cs Outdated Show resolved Hide resolved
LeaderboardBackend.Test/Leaderboards.cs Outdated Show resolved Hide resolved
LeaderboardBackend.Test/Leaderboards.cs Outdated Show resolved Hide resolved
LeaderboardBackend.Test/Leaderboards.cs Outdated Show resolved Hide resolved
LeaderboardBackend/Services/Impl/LeaderboardService.cs Outdated Show resolved Hide resolved
LeaderboardBackend/Controllers/LeaderboardsController.cs Outdated Show resolved Hide resolved
@zysim
Copy link
Collaborator Author

zysim commented Oct 17, 2024

I think we forgot to cover the case where the board can't be restored because its slug has been reclaimed by another board. Shall we simply have it return a 409 and possibly the conflicting board or at least the ID of the conflicting board?

409'ing sounds fair. I should add that in. I'll return the full board; better visibility that way.

@Eein
Copy link
Contributor

Eein commented Oct 17, 2024

I think we forgot to cover the case where the board can't be restored because its slug has been reclaimed by another board. Shall we simply have it return a 409 and possibly the conflicting board or at least the ID of the conflicting board?

409'ing sounds fair. I should add that in. I'll return the full board; better visibility that way.

Is this even possible?
If a leaderboard is deleted, it should still retain the unique slug index no?

@TheTedder
Copy link
Contributor

I think we forgot to cover the case where the board can't be restored because its slug has been reclaimed by another board. Shall we simply have it return a 409 and possibly the conflicting board or at least the ID of the conflicting board?

409'ing sounds fair. I should add that in. I'll return the full board; better visibility that way.

Is this even possible?
If a leaderboard is deleted, it should still retain the unique index no?

We decided that deleted LBs should get excluded from the unique index for slugs. This is achieved by filtering the index to only cover LBs that are not deleted. This means that when you delete an LB, its slug is freed. Alternatively, we could ask for a slug to use when restoring the LB to prevent the annoying and confusing case of having to give a deleted LB new slug before you restore it. In addition or perhaps instead, we could have it so that deleting a board automatically clears its slug.

@zysim
Copy link
Collaborator Author

zysim commented Oct 19, 2024

Reflecting what I've said on Discord; I won't be implementing the ability to pass a new slug in to restore a board, nor am I clearing a board's slug on deletion. If a board conflicts, it conflicts.

@zysim zysim requested a review from TheTedder October 19, 2024 15:54
LeaderboardBackend.Test/Leaderboards.cs Outdated Show resolved Hide resolved
LeaderboardBackend.Test/Leaderboards.cs Outdated Show resolved Hide resolved
LeaderboardBackend.Test/Leaderboards.cs Show resolved Hide resolved
LeaderboardBackend.Test/Leaderboards.cs Outdated Show resolved Hide resolved
LeaderboardBackend.Test/Leaderboards.cs Outdated Show resolved Hide resolved
LeaderboardBackend/Controllers/LeaderboardsController.cs Outdated Show resolved Hide resolved
@zysim zysim force-pushed the restoreLeaderboards branch from 3418ea1 to eb1b053 Compare October 20, 2024 13:16
@zysim zysim requested a review from TheTedder October 20, 2024 13:17
LeaderboardBackend/Controllers/LeaderboardsController.cs Outdated Show resolved Hide resolved
LeaderboardBackend/Controllers/LeaderboardsController.cs Outdated Show resolved Hide resolved
LeaderboardBackend.Test/Leaderboards.cs Outdated Show resolved Hide resolved
LeaderboardBackend/Services/Impl/LeaderboardService.cs Outdated Show resolved Hide resolved
LeaderboardBackend.Test/Leaderboards.cs Outdated Show resolved Hide resolved
LeaderboardBackend.Test/Leaderboards.cs Outdated Show resolved Hide resolved
LeaderboardBackend/Services/Impl/LeaderboardService.cs Outdated Show resolved Hide resolved
TheTedder
TheTedder previously approved these changes Oct 23, 2024
@zysim zysim merged commit ac3f493 into leaderboardsgg:main Oct 23, 2024
2 checks passed
@zysim zysim deleted the restoreLeaderboards branch October 23, 2024 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Restore Leaderboard Endpoint
3 participants