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

Break CRUD routes #703

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

Break CRUD routes #703

wants to merge 9 commits into from

Conversation

nruia-penn
Copy link
Contributor

@nruia-penn nruia-penn commented Feb 16, 2025

This pr adds a break view set so that someone can create and update breaks on PCP in the backend.

This is a simple view set with CRUD routes that should let a user add breaks to their schedule in penn course plan.

I wrote test cases in test_schedule.py.

Some design decisions:

  • I changed the meeting model to have either an associated Section or associated break but never both, this is so breaks can also have a location. If we choose not to use location (this could be hard to implement on front end with how many rooms there are in this school), there is also an option for a simple location_string, which can be set by the user.
  • The break model was put in plan/models.py not plan/courses.py, this makes more sense to me as it is a model only used by PCP.

@nruia-penn nruia-penn requested review from Clue88 and el-agua February 16, 2025 15:10
Copy link
Contributor

@el-agua el-agua left a comment

Choose a reason for hiding this comment

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

Good work! Just left a few comments to look at.


associated_break = models.ForeignKey(
"plan.Break",
null=True,
on_delete=models.CASCADE,
related_name="meetings",
help_text="The Section object to which this class meeting belongs.",
Copy link
Contributor

Choose a reason for hiding this comment

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

"Meeting object"

@@ -1180,6 +1190,19 @@ class Meeting(models.Model):
),
)

def clean(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider using Django's built in CONSTRAINT libraries. It is probably more natural to SQL
https://docs.djangoproject.com/en/5.1/ref/models/constraints/

@nruia-penn nruia-penn requested a review from el-agua February 17, 2025 01:18
Copy link
Contributor

@el-agua el-agua left a comment

Choose a reason for hiding this comment

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

Small thing, but looking good!

context = super().get_serializer_context()
include_location_str = "False"
# TODO: figure out how we want to do locations.
context.update({"include_location": eval(include_location_str)})
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets try not to use eval, if locations are not considered in this v1 version, we can comment out?

https://www.udacity.com/blog/2023/03/pythons-eval-the-most-powerful-function-you-should-never-use.html

@nruia-penn
Copy link
Contributor Author

I think work now?

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