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

Initial Waitlisting Feature Dev #506

Merged
merged 13 commits into from
Nov 19, 2024

Conversation

gabrielhan23
Copy link

Finished initial backend feature for waitlisting.

  • waitlist model (using timestamps)
  • add and drop api for waitlist
  • auto add from waitlist after students drop
  • light testing with api and admin tools

Not implemented yet:

  • coord capabilities
  • what to do when student being added from waitlist fails
  • waitlist ordering

@gabrielhan23 gabrielhan23 changed the base branch from master to feat/waitlisting-fa24/main November 1, 2024 16:01
Copy link
Member

@smartspot2 smartspot2 left a comment

Choose a reason for hiding this comment

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

Haven't taken too close of a look at the implementation yet, but just wanted to point out some things I noticed; left some comments about them.

In addition, try to at least get the pytests and the pre-commit check to pass (I don't believe that they'll automatically run on this PR since it's not targeting master, so you may need to manually re-run tests on the PR).

anonymized_data.json Outdated Show resolved Hide resolved
docker-compose.yml Outdated Show resolved Hide resolved
Copy link

@faithdennis faithdennis left a comment

Choose a reason for hiding this comment

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

Some tests seem to be failing according to the automated tests. Could changing section view have broken other tests? If not, I'm not sure if these automated tests are valid.

@smartspot2
Copy link
Member

smartspot2 commented Nov 5, 2024

Some tests seem to be failing according to the automated tests. Could changing section view have broken other tests? If not, I'm not sure if these automated tests are valid.

pre-commit is failing due to the anonymized_data.json file being incorrectly committed (along with some python style issues), and pytest/cypress tests are failing due to the following error:

Traceback (most recent call last):

  File "/opt/hostedtoolcache/Python/3.9.20/x64/lib/python3.9/site-packages/django/db/backends/utils.py", line 89, in _execute

return self.cursor.execute(sql, params)
psycopg2.errors.NotNullViolation: null value in column "max_waitlist" of relation "scheduler_course" violates not-null constraint

DETAIL:  Failing row contains (1, CS61A, 2024-12-01, 2024-10-02 16:03:44.763658+00, 2024-11-16 17:03:44.763658+00, 2, Structure and Interpretation of Computer Programs, 2024-10-17, f, null, null).

Seems like you need to make sure that max_waitlist is set for all Course models in tests, since it's set as non-null.

@gabrielhan23 gabrielhan23 merged commit ed65782 into feat/waitlisting-fa24/main Nov 19, 2024
@gabrielhan23 gabrielhan23 deleted the feat/waitlisting-fa24/dev branch November 19, 2024 02:48
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.

4 participants