Pcp schedule sharing (with flushed out views and urls)

[anshnagwekar]

See comments in code for any clarification!

[mureytasroc]

Nice work - feel free to ignore anything if it's too pedantic (currently experiencing the annoyance of a too-pedantic reviewer coworker at work and it can be counterproductive imo -- but also I learn a lot from their comments so I think at least seeing the comments is a benefit).

[mureytasroc]

Great progress, really clean code. Just some suggestions to make things more Django-friendly / utilize built-in functionality.

[mureytasroc]

Sorry ik this is an annoying comment, but usually we want to test code from the API interface, rather than testing specific implementation details (like the model). This makes the tests less brittle / less of "change indicators". It also provides more comprehensive testing, as lots of our interesting logic is in the viewset, not just the model. Issues with the model will surface if we properly test the code from the API level.

The schedules test are a good example of this:
https://github.com/pennlabs/penn-courses/blob/master/backend/tests/plan/test_schedule.py

You can actually send test requests e.g.:

sender = User.objects.create_user(
    username="sender", email="[email protected]", password="top_secret"
)
recipient = User.objects.create_user(
    username="recipient", email="[email protected]", password="top_secret"
)
self.client.login(username="sender", password="top_secret")
response_sent = self.client.post(
    "/api/plan/friendships/",
    json.dumps({"friend_id": recipient.id}),
    content_type="application/json",
)
# TODO: check response_sent for correctness, e.g. status code, content
recipient_client = APIClient()
recipient_client.login(username="recipient", password="top_secret")
response_accepted = recipient_client.put(
    "/api/plan/friendships/",
    json.dumps({"friend_id": sender.id}),
    content_type="application/json",
)
# TODO: check response_accepted for correctness, e.g. status code, content

[mureytasroc]

Nice work. We're super close with this. Two high level comments:

• You might want to cut down on the amount of in-code comments that are just describing what the code does. Comments should be thought of as a shortcut - if you are commenting a complicated block of code to allow your reader to skip over that block (by just reading your comment), that's valuable. On the other hand if you have one line of comment describing one line of simple code, it's just adding bloat / taking up unnecessary space imo. Cleaner to just write good code and let it speak for yourself. The other situation where comments are valuable is when you have complicated invariants or implicit assumptions. Anything that can't be easily figured out from the code itself.
• I think sometimes you add a bit too much bloat to your JSON responses. Status messages are useful for error cases (to tell the user what went wrong), and sometimes it's useful to include some data in the content of your response to an action (e.g. I think returning the state of the friendship object after the POST friendship action has been executed is valuable). But in most cases if you are executing an action method (e.g. anything other than GET), you probably don't need to return a response json at all. The user already knows what they sent you (e.g. what resource they are POSTing, in the case of PrimarySchedule), and your status code should speak for itself.

[mureytasroc]

LGTM so far