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

feat: add view for allowance creation #291

Merged
merged 1 commit into from
Jul 22, 2024
Merged

Conversation

alangsto
Copy link
Member

@alangsto alangsto commented Jul 22, 2024

JIRA: COSMO-369

Description: This PR adds the ability to create one or more allowances via a POST request.

Merge checklist:

  • All reviewers approved
  • CI build is green
  • Changelog record added
  • Documentation updated (not only docstrings)
  • Commits are squashed

Post merge:

  • Delete working branch (if not needed anymore)

@alangsto alangsto force-pushed the alangsto/allowances_post branch from b3bc7e6 to 4d20e9b Compare July 22, 2024 13:21
Copy link

github-actions bot commented Jul 22, 2024

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  edx_exams/apps/api
  serializers.py
  edx_exams/apps/api/v1
  views.py
  edx_exams/apps/api/v1/tests
  test_views.py
Project Total  

This report was generated by python-coverage-comment-action

@alangsto alangsto force-pushed the alangsto/allowances_post branch 3 times, most recently from 02a8463 to 0ce433a Compare July 22, 2024 13:54

def test_invalid_post(self):
"""
Test that 400 response is returned if serializer is invalid
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a test where none of the expected query strings are provided? No exam_id, no user_id, and no extra_time_mins? What about test that some querystrings are missing?

Copy link
Member

@schenedx schenedx left a comment

Choose a reason for hiding this comment

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

👍

@alangsto alangsto force-pushed the alangsto/allowances_post branch 5 times, most recently from eb0f619 to 516f42c Compare July 22, 2024 17:34
if allowance.get('username')
else User.objects.get(email=allowance['email'])
),
exam=Exam.objects.get(id=allowance['exam_id']),
Copy link
Contributor

@zacharis278 zacharis278 Jul 22, 2024

Choose a reason for hiding this comment

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

Normally I'd say don't query inside a loop but it's probably okay here. May be a waste of complexity optimizing it. It could be worth at least commenting that we expect the number of allowances to be reasonably small.

Copy link
Contributor

@zacharis278 zacharis278 left a comment

Choose a reason for hiding this comment

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

👍

@alangsto alangsto force-pushed the alangsto/allowances_post branch from 516f42c to f12bceb Compare July 22, 2024 18:00
@alangsto alangsto force-pushed the alangsto/allowances_post branch from f12bceb to f3c4d89 Compare July 22, 2024 18:00
@alangsto alangsto merged commit 0d1c2af into main Jul 22, 2024
5 checks passed
@alangsto alangsto deleted the alangsto/allowances_post branch July 22, 2024 18:18
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.

3 participants