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

Onboarding backend #624

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

Onboarding backend #624

wants to merge 12 commits into from

Conversation

ashleymyan
Copy link

• Added DegreeProfile and CourseTaken models to store onboarding info
• Basic CRUD routes, serializers, and unit tests
• Transcript model and transfer credit attribute are there but not yet implemented

@ashleymyan ashleymyan requested a review from AaDalal April 7, 2024 17:05
Copy link
Contributor

@AaDalal AaDalal left a comment

Choose a reason for hiding this comment

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

Overall pretty great job using Django/Django rest framework concepts! I left some comments. I think the biggest thing to think about is what is the minimum information we need to store? And how can we use that on the frontend?

Copy link
Contributor

Choose a reason for hiding this comment

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

heads up we use yarn, so shouldn't need a package-lock.json since we use yarn

@@ -0,0 +1,38 @@
# Generated by Django 3.2.24 on 2024-03-24 21:30
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: it's a good idea to merge these migrations into one (you can just roll back to the migration before the ones you added using python manage.py migrate XXXX where XXXX is the last migration not in this PR. You can then delete all the migrations created in this PR and run python manage.py makemigration



class DegreeProfile(models.Model):
user_profile = models.OneToOneField(
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we just foreign key directly to the User? is there a reason not to?

help_text="extending the user profile class",
)

transcript = models.OneToOneField(
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we want this field (ie, we shouldn't store the transcript)

),
)

transfer_credits = models.ManyToManyField(
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this might need to be an array of full_codes? Courses are only the ones we have in our database, but PDP supports other course codes, e.g., EAS 0091 which is not an official course according to our db

model = Degree
fields = "__all__"

def validate_degrees(self, degrees):
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this? I think this check should be handled by the view level (the view defines a get_object that handles this)

),
)

degrees = models.ManyToManyField(
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we store degrees here? How would we display this on the onboarding panel.

),
)

def calculate_total_credits(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we planning on using this anywhere?

user_profile, _ = UserProfile.objects.get_or_create(user=self.request.user)
return serializer.save(user_profile=user_profile.id)

def update(self, request, *args, **kwargs):
Copy link
Contributor

Choose a reason for hiding this comment

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

if we have a ModelViewset, the nice part is we don't have to define most of these things. See e.g., the SectionList view. This is because django rest framework ModelViewset already has defaults for these things.

return super().destroy(request, *args, **kwargs)

@action(detail=True, methods=["post"], url_path="add-course", url_name="add_course")
def add_course(self, request, pk=None):
Copy link
Contributor

Choose a reason for hiding this comment

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

This is nice, I like this pattern for adding/removing on manytomany fields

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants