-
Notifications
You must be signed in to change notification settings - Fork 5
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
IEEE-21 /api/event/teams/team #280
Conversation
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.
See comments.
I also think your version of Black might be wrong, since some of the formatting doesn't agree. Run black --version
, and make sure you're on version 19.10b0
. The later versions change formatting habits a bit it seems.
@@ -10,7 +10,20 @@ class Meta: | |||
fields = ("id", "name") | |||
|
|||
|
|||
class TeamSerializer(serializers.ModelSerializer): |
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.
It wasn't in the ticket, but I think we should add a profiles serializer into here. That way, on the frontend, we can just get /api/event/teams/team/
, and we'll have all the information we need about the team (aka, the members on it), rather than having to make a separate request.
When we eventually have a list view of /api/event/teams/
for the admin side of the site, that will also come in handy so we don't have to fetch the list of team members separately.
To do that, you won't want to use the ProfileSerializer
below, since that includes the team - so you'll end up with a circular reference. I would create a new ProfileInTeamSerializer
, that includes the profile as well as important related user fields - first name, last name, email. I think you can just put user__first_name
etc in the fields list, otherwise you'll need a user serializer too.
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.
Actually, disregard this for now. I'll create a separate ticket, so that we don't get in the habit of scope creep (which I am bad for).
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.
cool, is it ok if I resolve this?
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.
Let's keep it unresolved just so it's visible for future reference
I'm having issues installing Black with that version. For example, "npm install black==19.10b0" or "yarn install black==19.10.0". I also tried "i" instead of install and also a decimal instead of b. Nothing seems to work. Other than that, everything seems ok |
hey, so I still don’t fully understand the error the test function that is bringing up the error is
The error it brings up is the same as the one on GitHub Actions. My issue is the error still persists after adding the import statements shown below and changing the usages of the word “Team” to “EventTeam” in event/tests.py.
I didn't see any of the team imports (other than the word TeamSerializer, which we aren't using in the function above) be greyed out. Furthermore, it seems like when I use I tried changing the event/views.py code to include a similar input I've committed my code, if you could let me know what am I missing in my analysis, it would be much appreciated. |
Overview
Unit Tests Created
Steps to QA