Conversation
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR introduces client ID registration, adding a new Client model field, endpoint, serializer, email notification, and supporting infrastructure.
- Add
Client.client_idauto-generation and migrations to support unique IDs - Create
ClientSerializerwith full validation and views for registration and validation, including email notifications - Update settings, Docker, and CI configs to support database-driven registration endpoints
Reviewed Changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| rorapi/v2/tests.py | Add model tests for client ID creation and rate limiting |
| rorapi/v2/serializers.py | Introduce ClientSerializer with validation for all fields |
| rorapi/v2/models.py | Add Client model with client_id generation on save |
| rorapi/common/views.py | Add ClientRegistrationView and ValidateClientView |
| rorapi/common/urls.py | Wire up registration and validation routes |
| rorapi/settings.py | Conditional DB config for collectstatic, toggle rate limiting |
| rorapi/management/commands/generaterorid.py | Add helper to generate random ROR client IDs |
| README.md | Document credentials and setup for client ID registration |
Comments suppressed due to low confidence (8)
rorapi/common/views.py:48
- No tests cover the
ClientRegistrationViewendpoint or its email-sending side effects. Add integration tests to verify 201 responses, validclient_idin the payload, and that an email is queued or sent.
class ClientRegistrationView(APIView):
rorapi/v2/models.py:164
- The current implementation may generate duplicate IDs, causing a unique constraint error on save. Consider looping (with a max retry count) to regenerate until a truly unique
client_idis produced.
def generate_client_id():
rorapi/management/commands/generaterorid.py:30
- The function references
base32_crockfordbut there is no import for it; this will cause a NameError at runtime. Addimport base32_crockfordat the top of the module.
def generate_ror_client_id():
rorapi/settings.py:295
- [nitpick] Duplicate
import os—remove the redundant import to keep imports clean.
import os
rorapi/settings.py:117
- [nitpick] Indentation of the
'default'key is inconsistent inside theDATABASESdict, making the block harder to read—align the braces and contents.
'default': {
rorapi/common/urls.py:17
- The
registerendpoint omits a trailing slash, while other paths include or omit slashes inconsistently. Standardize URL patterns (e.g., always include a trailing slash) for consistency.
re_path(r"^(?P<version>(v1|v2))\/register$", ClientRegistrationView.as_view()),
README.md:27
- [nitpick] Consistently capitalize 'GitHub' instead of 'Github' to match project branding and external references.
but should comment out this line https://github.com/ror-community/ror-api/blob/8a5a5ae8b483564c966a7184349c581dcae756ef/rorapi/management/commands/setup.py#L13 so that there is no attempt to send a Github token when retrieving a data dump for indexing.
rorapi/common/views.py:70
- [nitpick] The multi-line f-string includes leading indentation in the email body. Consider using
textwrap.dedentor building the string without extra spaces to avoid unwanted whitespace in sent emails.
return f"""
jrhoads
approved these changes
May 15, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.