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

add compete data to teaminfo #558

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Conversation

apollo1291
Copy link
Contributor

draft: My staging is messed up so Ihave not tested. Because I cant test, I'm not refactoring the api file, more focused on functionality for the deadline. This pr changes teaminfo, just from comments. Should I add win-loss to the teamcard component?

@apollo1291 apollo1291 linked an issue Jan 15, 2023 that may be closed by this pull request
@apollo1291 apollo1291 requested a review from n8kim1 January 15, 2023 19:15
@n8kim1
Copy link
Contributor

n8kim1 commented Jan 15, 2023

Rank and PerfCard wouldn't be ready, cuz backend doesn't exist yet for that :// Backend doesn't (yet) expose rank, totals wins, or total losses. We should work on that first! (You could, thought you'd have to set up first...)
Showing rating would be doable now, although seems more efficient to do later too.

So in all, if you could get backend set up, then maybe work on the backend side first? Otherwise just hold off on this and find another cool frontend issue ://

@gaurav-arya
Copy link
Contributor

gaurav-arya commented Jan 15, 2023

I believe we'd previously decided that total wins and total losses should be computed on frontend based on team history (similar to what's already being done, but adapted to the new endpoint) in which case backend should be already ready. But perhaps that decision has changed recently? @j-mao any input?

@n8kim1
Copy link
Contributor

n8kim1 commented Jan 15, 2023 via email

@apollo1291 apollo1291 marked this pull request as ready for review January 24, 2023 23:19
@apollo1291 apollo1291 requested a review from n8kim1 January 24, 2023 23:19
@n8kim1 n8kim1 force-pushed the compete-based-teamcard branch from 1485f0d to 6faed96 Compare January 25, 2023 17:59
@n8kim1 n8kim1 force-pushed the compete-based-teamcard branch from 6faed96 to 48d62e2 Compare January 25, 2023 19:53
Copy link
Contributor

@n8kim1 n8kim1 left a comment

Choose a reason for hiding this comment

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

looks great; just a few small changes. but base of code is good

@apollo1291 apollo1291 requested a review from n8kim1 January 25, 2023 21:18
@n8kim1 n8kim1 force-pushed the compete-based-teamcard branch from 95cb175 to d3c809a Compare January 25, 2023 21:19
Copy link
Member

@j-mao j-mao left a comment

Choose a reason for hiding this comment

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

Why the new route in urls?

detail=False,
methods=["post"],
serializer_class=None,
permission_classes=(IsAuthenticated, IsEpisodeAvailable),
Copy link
Member

Choose a reason for hiding this comment

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

I might suggest removing IsAuthenticated, to allow this to be viewable by the public


@action(
detail=False,
methods=["post"],
Copy link
Member

Choose a reason for hiding this comment

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

This should probably be get, since it’s a non mutator.

Copy link
Member

Choose a reason for hiding this comment

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

You need detail = True to get pk

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.

Adapt compete-based data on public team card to new frontend
4 participants