Created models for penn wrapped -- issue with pip files though #309

zachHarpaz · 2024-09-29T14:20:48Z

No description provided.

request should look like this format: /wrapped/generate/?username=zharpaz&semester=2020A

vcai122

Good work so far! Django has a lot of intricacies but you're doing a good job navigating them so keep up the good work. Let me know if you have any questions about comments

vcai122 · 2024-10-20T18:54:29Z

backend/wrapped/models.py

+
+
+class GlobalStat(models.Model):
+


You don't need to prefix with stat_ because it's a bit redundant

vcai122 · 2024-10-20T18:54:53Z

backend/wrapped/models.py

+
+
+# Add a new model for keys 
+class UserStatKey(models.Model):


Let's keep the names consistent and call this IndividualStatKey

vcai122 · 2024-10-20T19:01:33Z

backend/wrapped/models.py

+
+# Add a new model for keys 
+class UserStatKey(models.Model):
+    stat_key = models.CharField(max_length=50, null=False, blank=False)


same here about stat_ prefix. Also make this the primary key

vcai122 · 2024-10-20T19:32:50Z

backend/wrapped/models.py

+    # How to do this, using individual vs stat_key ?
+    name = models.CharField(max_length=50, null=False, blank=False)
+    template_path = models.CharField(max_length=50, null=False, blank=False)
+    stat_locations = models.ManyToManyField(StatLocation, blank=True)


let's make this a through model see Gsr Group to User through GroupMembership as an example.

Also let's not make this "one or the other" model. Let's just make the administrator specify whether it would be individual or global through 2 seperate many to manys. It seems reasonable for the administrator to think about this anyway.

vcai122 · 2024-10-20T19:36:54Z

backend/wrapped/models.py

+class Page(models.Model):
+
+    # How to do this, using individual vs stat_key ?
+    name = models.CharField(max_length=50, null=False, blank=False)


This name should be for internal use only. Also make it a primary key to ensure uniqueness.

vcai122 · 2024-10-20T19:41:59Z

backend/wrapped/views.py

+User = get_user_model()
+
+
+class GeneratePage(viewsets.ModelViewSet): 


get the user through request.user, and make sure you have to be authenticated to use this route. see our other views.

Also I don't think ModelViewSet is the right viewset to use here.

Also call this and associated urls Pages. We're not really generating anything (generating implies side effects). We're just serializing data already in the db: we are returning Pages filtered for the given semester, serialized in a particular way.

Actually now that we are adding a Semester model, this should be thought of as returning data associated with 1 particular semester.

(And in the serializer for semester it turns it into a list of pages each of which can be serialized with the page serializer and so on...)

EDIT: actually I'm not partial to either, I think thinking about it as returning a list of pages seems fine too.

vcai122 · 2024-10-20T19:42:44Z

backend/wrapped/views.py

+        pages = Page.objects.filter(semester=semester)
+
+        # Serialize the data
+        serializer = PageSerializer(pages, many=True)


what does the many=True here do

vcai122 · 2024-10-20T19:44:33Z

backend/wrapped/models.py

@@ -0,0 +1,76 @@
+from django.db import models


I know we talked about this before but I think we should actually make Semester a model again.
Reason I'm saying this is for admin page purposes, we can use inline models to manage all the pages for a particular semester at a time which seems like a clean way to do it.

We would make a semester model with 1 field, and make it many to many on pages.

We should think of a Page from now on as a page reusable across semesters

Leaving this here because we should probably use this later: https://django-admin-sortable2.readthedocs.io/en/latest/

So you have to also make GlobalStatKey model

vcai122 · 2024-10-20T19:48:15Z

backend/wrapped/serializers.py

+        model = UserStatKey
+        fields = ["stat_key"]
+
+class PageSerializer(serializers.ModelSerializer):


You're not really using the other serializers since you're just serializing it and immediately reformatting it, so might as well not use it.

The way you should be thinking about serializers is that everytime you have a queryset and then you say to yourself "I wish I could have this queryset in this particular format" you should make a serializer.

Finished Models for wrapped and serializer working

…into penn-wrapped

vcai122

Looking very promising! Next steps would be to clean up a lot of the dead code and get it ready to merge.

Most comments I left should be pretty quick and so let's just get this into a ready to merge state soon.

vcai122 · 2024-11-09T18:47:33Z

backend/wrapped/models.py

+
+
+# Why do we want time? Ask Vincent 
+class Page(models.Model):


lets move this to the bottom so we don't have to specify the many to many models using strings

vcai122 · 2024-11-09T18:48:23Z

backend/wrapped/models.py

+
+
+# Why do we want time? Ask Vincent 
+class Page(models.Model):


also we need a duration field indicating how long this page should stay on screen for. This would be allowed to be empty

vcai122 · 2024-11-09T18:49:17Z

backend/wrapped/models.py

+
+
+class IndividualStatThrough(models.Model):
+    IndividualStatKey = models.ForeignKey(IndividualStatKey,null=False, blank=False, default=None ,on_delete=models.CASCADE)


is there a reason this is IndividualStatKey instead of individual_stat_key? same with others below

vcai122 · 2024-11-09T18:50:58Z

backend/wrapped/models.py

+
+
+
+


Let's call this something more descriptive to its actual inherent meaning. Maybe IndividualStatPageField (and the other GlobalStatPageField

vcai122 · 2024-11-09T18:57:24Z

backend/wrapped/views.py

+from .models import Page, Semester
+from .serializers import PageSerializer
+from .serializers import SemesterSerializer
+


Now that your doing it the django way, you can get away with writing even less code :)
e.g. something like this

class MyModelViewSet(viewsets.ReadOnlyModelViewSet): queryset = MyModel.objects.all() serializer_class = MyModelSerializer

Note you don't need to pass in user in context. See how we access the user in serializers in our other code (e.g. Portal serializer)

vcai122 · 2024-11-09T19:05:40Z

backend/wrapped/serializers.py

+        model = Semester
+        fields = ['semester', 'pages']
+
+    def get_pages(self, obj):


You are manually writing what django can already do if you just hook up pages directly with the serializer. (See sublet serializers for example).
e.g.

amenities = serializers.PrimaryKeyRelatedField( many=True, queryset=Amenity.objects.all(), required=False ) images = SubletImageURLSerializer(many=True, required=False)

wait sorry i think for this you need to manually pass the semester obj so yours is correct. But its kinda verbose why not just:

return PageSerializer(obj.pages.all(), many=True, context={'semester': obj}).data

vcai122 · 2024-11-09T19:30:34Z

backend/wrapped/serializers.py

+from .models import Page, IndividualStat, GlobalStat, IndividualStatThrough, GlobalStatThrough, Semester, User
+
+class IndividualStatSerializer(serializers.ModelSerializer):
+    key = serializers.StringRelatedField()


Let's not use StringRelatedField because it relies on the __str__ which is ususally used for testing. Can you find a better one to use?

vcai122 · 2024-11-09T19:43:08Z

backend/wrapped/serializers.py

+        user = self.context.get('user')
+        semester = self.context.get('semester')
+
+        individual_throughs = IndividualStatThrough.objects.filter(Page=obj)


You a bit too much manual serialization here. As an outline the following is a bit more reasonable:

Serialize IndividualEntries to become a list of (field_name, value) pairs (using a serializer)
2.Serialize IndividualEntries to become a list of (field_name, value) pairs (using a serializer)

Combine into a dict and return.

Although this seems hard to describe over a code review. Maybe we can sit down and peer program for this part

vcai122 · 2024-11-09T19:45:03Z

backend/wrapped/serializers.py

+        user = self.context.get('user')
+        semester = self.context.get('semester')
+
+        try:


prefer filter(...).first() over get()

codecov · 2024-11-15T06:11:08Z

Codecov Report

Attention: Patch coverage is 82.57576% with 23 lines in your changes missing coverage. Please review.

Project coverage is 90.23%. Comparing base (8315c52) to head (5408760).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
backend/wrapped/serializers.py	68.42%	12 Missing ⚠️
backend/wrapped/models.py	86.20%	8 Missing ⚠️
backend/wrapped/views.py	72.72%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #309      +/-   ##
==========================================
- Coverage   90.59%   90.23%   -0.36%     
==========================================
  Files          61       66       +5     
  Lines        2552     2684     +132     
==========================================
+ Hits         2312     2422     +110     
- Misses        240      262      +22

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

vcai122

LGTM!
Notes before merging:

Merge in master to get rid of build errors
Make sure everything is passing (except for test coverage)
Note down the decrease in code coverage, you'll be responsible for bringing it up in ur next PR

… penn-wrapped

Created models

f14d390

zachHarpaz requested a review from vcai122 September 29, 2024 14:20

zachHarpaz added 2 commits September 29, 2024 12:08

pip file fix

e0dbbff

Fixed models.py and added to admin.py

3c8596f

tuneerroy marked this pull request as draft September 29, 2024 18:13

zachHarpaz and others added 3 commits October 13, 2024 12:53

models

5d725aa

Penn-Wrapped Backend Requests

c65cd62

request should look like this format: /wrapped/generate/?username=zharpaz&semester=2020A

fix github pipfile diff for no reason + others

eb43f14

vcai122 requested changes Oct 20, 2024

View reviewed changes

zachHarpaz added 2 commits November 8, 2024 14:07

Updated Models

2786e10

Finished Models for wrapped and serializer working

Merge branch 'penn-wrapped' of https://github.com/pennlabs/penn-mobile …

5ec9cf9

…into penn-wrapped

zachHarpaz requested a review from vcai122 November 9, 2024 17:54

vcai122 requested changes Nov 9, 2024

View reviewed changes

zachHarpaz added 2 commits November 10, 2024 12:17

Fixes

cad94c5

Penn-Wrapped Prod Push

775468f

zachHarpaz requested a review from vcai122 November 15, 2024 13:25

vcai122 approved these changes Nov 17, 2024

View reviewed changes

Merge branch 'master' of https://github.com/pennlabs/penn-mobile into…

5408760

… penn-wrapped

zachHarpaz marked this pull request as ready for review November 17, 2024 17:45

zachHarpaz merged commit d1044c2 into master Nov 17, 2024
7 of 9 checks passed

zachHarpaz deleted the penn-wrapped branch November 17, 2024 17:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Created models for penn wrapped -- issue with pip files though #309

Created models for penn wrapped -- issue with pip files though #309

zachHarpaz commented Sep 29, 2024

vcai122 left a comment

vcai122 Oct 20, 2024

vcai122 Oct 20, 2024

vcai122 Oct 20, 2024

vcai122 Oct 20, 2024

vcai122 Oct 20, 2024

vcai122 Oct 20, 2024

vcai122 Oct 20, 2024 •

edited

Loading

vcai122 Oct 20, 2024

vcai122 Oct 20, 2024

vcai122 Oct 20, 2024

vcai122 Oct 20, 2024

vcai122 Oct 20, 2024

vcai122 left a comment

vcai122 Nov 9, 2024

vcai122 Nov 9, 2024

vcai122 Nov 9, 2024

vcai122 Nov 9, 2024

vcai122 Nov 9, 2024

vcai122 Nov 9, 2024

vcai122 Nov 9, 2024

vcai122 Nov 9, 2024

vcai122 Nov 9, 2024

vcai122 Nov 9, 2024

vcai122 Nov 9, 2024

codecov bot commented Nov 15, 2024 •

edited

Loading

vcai122 left a comment

		User = get_user_model()


		class GeneratePage(viewsets.ModelViewSet):



		class IndividualStatThrough(models.Model):
		IndividualStatKey = models.ForeignKey(IndividualStatKey,null=False, blank=False, default=None ,on_delete=models.CASCADE)

Created models for penn wrapped -- issue with pip files though #309

Created models for penn wrapped -- issue with pip files though #309

Conversation

zachHarpaz commented Sep 29, 2024

vcai122 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vcai122 Oct 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vcai122 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 15, 2024 • edited Loading

Codecov Report

vcai122 left a comment

Choose a reason for hiding this comment

vcai122 Oct 20, 2024 •

edited

Loading

codecov bot commented Nov 15, 2024 •

edited

Loading