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

Update course members sections #41

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

abeeto
Copy link
Collaborator

@abeeto abeeto commented Apr 11, 2024

  • using action decorator to set up endpoint: api/v1/course-members/{id}/update-sections/
  • now validating the request.data and checks if:
    • is a list of numbers
    • does not accept a list of sections that are not in the course of the course-member
  • using transaction.atomic and action decorator to configure a endpoint to update course-member
  • the sections field is being validated through the use of a serializer made for this endpoint called UpdateStudentsSectionRequest in views/course_member.py
  • endpoint returns the same new student once a valid set of ids are returned.

@abeeto abeeto requested a review from Opeyem1a April 11, 2024 08:12
app/views/course_member.py Outdated Show resolved Hide resolved
app/views/course_member.py Outdated Show resolved Hide resolved
app/views/course_member.py Outdated Show resolved Hide resolved
app/views/course_member.py Outdated Show resolved Hide resolved
@abeeto abeeto requested a review from Opeyem1a April 13, 2024 06:26
app/serializers/student_sections_update.py Outdated Show resolved Hide resolved
app/serializers/student_sections_update.py Outdated Show resolved Hide resolved
app/serializers/student_sections_update.py Outdated Show resolved Hide resolved
app/serializers/student_sections_update.py Outdated Show resolved Hide resolved
app/serializers/student_sections_update.py Outdated Show resolved Hide resolved
app/serializers/student_sections_update.py Outdated Show resolved Hide resolved
app/views/course_member.py Outdated Show resolved Hide resolved
app/views/course_member.py Outdated Show resolved Hide resolved
app/views/course_member.py Outdated Show resolved Hide resolved
app/views/course_member.py Outdated Show resolved Hide resolved
@abeeto
Copy link
Collaborator Author

abeeto commented Apr 14, 2024

Sorry about the previous PR, it was late at night and I got way too greedy with GPT/copilot. Hope the changes make more sense now.

@abeeto abeeto requested a review from Opeyem1a April 14, 2024 15:43
Copy link
Member

@Opeyem1a Opeyem1a left a comment

Choose a reason for hiding this comment

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

this isn't the goal of your task, but idk this endpoint seems kinda messy. I would prefer to see an actual schema of what the response object looks like. An example is something like:

{
    "data": ...the new serialized course member after update,
    "error": ...an error object if there is one
}

app/views/course_member.py Outdated Show resolved Hide resolved
app/views/course_member.py Outdated Show resolved Hide resolved
context={"course": course_member.course},
)
if StudentSectionsRequestSerializer.is_valid(raise_exception=True):
section_ids = StudentSectionsRequestSerializer._validated_data.get(
Copy link
Member

Choose a reason for hiding this comment

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

fields that start with _ usually aren't meant to be accessed directly. Are you sure this is the correct pattern here?

I feel like there is a clean() method on serializers that returns the validated data

@abeeto abeeto requested a review from Opeyem1a May 7, 2024 06:53
app/views/course_member.py Outdated Show resolved Hide resolved
@abeeto abeeto requested a review from Opeyem1a May 11, 2024 03:59
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.

2 participants