Backend Feedback

[smartspot2]

I'm glad that you've been keeping up with the onboarding work, and I'm happy to see the progress; you're doing great so far. Here are a few things that stood out to me when perusing the repositories for the backend assignments; feel free to reply to this issue if you have any comments or questions about the feedback!

Scheduler Models

• It seems like you're using self.name for the string representation for all of these models, even when the models don't contain a name (ex. the Student model below); this should be changed to be a little bit more descriptive and to fit more with what makes the instance unique (i.e. include the most important field(s) in the model)

• CSM_TECH_ONBOARDING/backend/scheduler/models.py

  Lines 9 to 11 in d7bdd7a

  	class Student(models.Model):
  	user = models.ForeignKey(User, on_delete=models.PROTECT)
  	section_name = models.ManyToManyField('Section')

  The relationship between the Student model and the Section model is tricky; in the real scheduler, we don't have a many-to-many field between students and sections, even though it may seem that way.

  Instead, whenever a user signs up for multiple sections in different courses (we don't allow them to sign up for multiple sections in the same course), an entirely new Student instance is created, associated with the user. This way, every Student instance will always be associated with a single section.

• With the Section model, there are a few more things to point out:
  

  CSM_TECH_ONBOARDING/backend/scheduler/models.py

  Lines 25 to 30 in d7bdd7a

  	class Section(models.Model):
  	student_in_section = models.ManyToManyField(Student)
  	mentor = models.OneToOneField(Mentor, on_delete=models.CASCADE, primary_key=True)
  	capacity = models.IntegerField()
  	special = models.BooleanField(default=False)
  	name = models.CharField(max_length=200)

  • If you have a many-to-many relationship on Student, there is no need to specify the same in the section model; Django will automatically create reverse relationships.
  • Also, making the mentor field the primary key is interesting; while this would probably work, this would mean that changing the mentor for a section is hard—it'd change the identity of the section instance. Leaving the primary key be automatically generated as default would still be fine here (and is more conventional).
  • What is the special field for? I presume the name field is supposed to hold the section description.

Django Queries

• I've been told that you're still working on the query assignments, but can you please also add the query statements themselves to the file? The actual values themselves matter less than how you construct the queries.
• Completely optional, but if you want this to be formatted a little better, you can also use a markdown file rather than a text file; you can render code and actual bullet lists in a much cleaner format that way.

Views

• The model issues pointed out above break the createtestdata script, as the script was made with a specific structure in mind. The most important thing to change is the relationship between Student and Section; feel free to look at the script to see how the models are meant to be created (i.e. the .create() lines), and what kind of fields it expects (this should give you a lot of information about what the structure should look like):

  CSM_TECH_ONBOARDING/django_lab/app/management/commands/createtestdata.py

  Lines 11 to 57 in d7bdd7a

  	class Command(BaseCommand):
  	def handle(self, *args, **options):
  	fake = Faker()
  	users = []
  	# create users
  	for i in range(NUM_TO_CREATE):
  	first = fake.first_name()
  	last = fake.last_name()
  	user = User.objects.create(
  	username=f"{first}{last}",
  	email=f"{first}{last}@berkeley.edu",
  	first_name=first,
  	last_name=last,
  	)
  	users.append(user)
  	# create course
  	course = Course.objects.create(name="CS70")
  	# create mentors
  	mentors = []
  	potential_students = []
  	for i, user in enumerate(users):
  	is_mentor = (i == 0 or random.choice([True, False])) and len(mentors) < 3
  	if is_mentor:
  	mentor = Mentor.objects.create(user=user, course=course)
  	mentors.append(mentor)
  	else:
  	potential_students.append(user)
  	# create sections for each mentor
  	sections = []
  	for mentor in mentors:
  	section = Section.objects.create(mentor=mentor, capacity=10)
  	sections.append(section)
  	# create students
  	for user in potential_students:
  	section = random.choice(sections)
  	Student.objects.create(
  	user=user,
  	section=section,
  	course=section.mentor.course,
  	active=random.choice([True, False]),
  	banned=False,
  	)

• With the serializers, make sure that you're using the model serializer for everything; there's no need to make your own create and update methods, and it makes the code a lot shorter and more concise.

  CSM_TECH_ONBOARDING/django_lab/app/serializers.py

  Lines 15 to 22 in d7bdd7a

  	def create(self, validated_data):
  	return Attendance.objects.create(**validated_data)
  	def update(self, instance, validated_data):
  	instance.user_type = validated_data.get('user_type', instance.user_type)
  	instance.save()
  	return instance

• With the views, there are a couple things to point out:

  • There's no need to manually parse anything; Django already does all of it internally with pre-configured settings. You can just reference request.data to get the body of the request.
    

    CSM_TECH_ONBOARDING/django_lab/app/views.py

    Line 40 in d7bdd7a

    data = JSONParser().parse(request)

  • You're using the Django response objects, rather than the REST framework response objects. You should not have any imports from django in this file.
    

    CSM_TECH_ONBOARDING/django_lab/app/views.py

    Line 37 in d7bdd7a

    return JsonResponse(serializer.data, safe=False)

    In using the REST framework responses, you get a cool interface when you navigate to an endpoint, like below; you can verify that the REST framework responses are correct through this interface.

    

  • To specify the statuses, the REST framework provides a status object, which you've imported, but have not used:
    

    CSM_TECH_ONBOARDING/django_lab/app/views.py

    Line 6 in d7bdd7a

    from rest_framework import status

    In general, it's bad practice to use the raw numbers for responses; it's more readable to use the constants in the status object, as the name tells you what the response means.

• With registering URLs, notice how you're always prefixing everything with api/:
  

  CSM_TECH_ONBOARDING/django_lab/app/urls.py

  Lines 9 to 11 in d7bdd7a

  	path('api/users/', views.getAllUsers, name='users'),
  	path('api/students/', views.getAllStudents, name='students'),
  	path('api/mentors/', views.getAllMentors, name='mentors'),

  This can be made a lot easier (and more conventional) by registering the app urls under an api/ prefix in the parent urls file within django_lab; you've currently registered it under no prefix:
  

  CSM_TECH_ONBOARDING/django_lab/django_lab/urls.py

  Line 21 in d7bdd7a

  path('', include('app.urls')),

[kevinbu233]

Thank you so much for all the detailed comments! I really really appreciate them. I will look at them more closely first thing in the morning. I am wondering what should be my next steps. Do I fix all the issues and resubmit it back to you? Also, regarding the django queries, I have finished all the queries except one. I am lost on one specific question that concerns with "section that has zoom meetings with passwords". I did not see any information regarding passwords in the section models in the data set given. Please let me know if you have any further instructions. Once again, thank you so much!

[smartspot2]

You can fix the issues on your own time; just let me and @noortor know when you've done so, and we can take a look at things again. It also seems like you haven't done lab 4; it can be completed with the finished frontend lab 3 as a base, which Noor can take a closer look at, as he's responsible for the frontend onboarding (though he's quite busy recently with work).

With the zoom links, take a look at the data to see what the zoom link structure is, and how you can filter for that; it's not stored in the model directly. You can go into the Django console (python3 csm_web/manage.py shell_plus) and poke around the database to get more information about it.