-
-
Notifications
You must be signed in to change notification settings - Fork 96
Added nest app and slack command for sponsorship #947
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
base: main
Are you sure you want to change the base?
Conversation
07f5706
to
e7f54b1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good but needs to be structured properly first:
related_name="sponsorships", | ||
) | ||
|
||
class Meta: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move to top
backend/apps/slack/utils.py
Outdated
@@ -65,6 +83,48 @@ def get_news_data(limit=10, timeout=30): | |||
return items | |||
|
|||
|
|||
def get_or_create_issue(issue_link): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This scope is to broad for /sponsor command's function. Let's split it and remove from here.
backend/apps/slack/utils.py
Outdated
try: | ||
return Issue.objects.get(url=issue_link) | ||
except Issue.DoesNotExist: | ||
pass | ||
|
||
github_client = Github(settings.GITHUB_TOKEN) | ||
issue_number = int(parts[6]) | ||
owner = parts[3] | ||
repo_name = parts[4] | ||
|
||
try: | ||
# Fetch the repository and issue from GitHub | ||
gh_repo = github_client.get_repo(f"{owner}/{repo_name}") | ||
gh_issue = gh_repo.get_issue(issue_number) | ||
repository = Repository.objects.get(name=repo_name) | ||
author = User.objects.get(login=gh_issue.user.login) | ||
|
||
# Update or create the issue in the database | ||
return Issue.update_data(gh_issue, author=author, repository=repository) | ||
except Exception as e: | ||
logger.exception("Failed to fetch issue from GitHub: %s") | ||
raise ValidationError(FETCH_ISSUE_ERROR.format(error=e)) from e |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be a part of github.common.py
sync_issue()
backend/apps/slack/utils.py
Outdated
repository = Repository.objects.get(name=repo_name) | ||
author = User.objects.get(login=gh_issue.user.login) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These may not exist yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes what did you suggest create user and repository too ?(if it not exist in the database?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we should create them first.
backend/apps/slack/utils.py
Outdated
|
||
# Extract repository owner, repo name, and issue number from the issue link | ||
# Example: https://github.com/OWASP/Nest/issues/XYZ | ||
parts = issue_link.strip("/").split("/") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use urllib.parse.urlparse
for this.
backend/apps/slack/utils.py
Outdated
def validate_deadline(deadline_str): | ||
"""Validate that the deadline is in a valid datetime format.""" | ||
try: | ||
# Try parsing the deadline in YYYY-MM-DD format | ||
deadline = datetime.strptime(deadline_str, "%Y-%m-%d").replace( | ||
tzinfo=timezone.get_current_timezone() | ||
) | ||
except ValueError: | ||
try: | ||
# Try parsing the deadline in YYYY-MM-DD HH:MM format | ||
deadline = datetime.strptime(deadline_str, "%Y-%m-%d %H:%M").replace( | ||
tzinfo=timezone.get_current_timezone() | ||
) | ||
except ValueError as e: | ||
raise ValidationError(DEADLINE_FORMAT_ERROR) from e | ||
|
||
if deadline < timezone.now(): | ||
raise ValidationError(DEADLINE_FUTURE_ERROR) | ||
|
||
return deadline | ||
|
||
|
||
def validate_github_issue_link(issue_link): | ||
"""Validate that the issue link belongs to a valid OWASP-related repository.""" | ||
if not issue_link.startswith("https://github.com/OWASP"): | ||
raise ValidationError(ISSUE_LINK_ERROR) | ||
return issue_link | ||
|
||
|
||
def validate_price(price): | ||
"""Validate that the price is a positive float value.""" | ||
try: | ||
price = float(price) | ||
if price <= 0: | ||
raise ValidationError(PRICE_POSITIVE_ERROR) | ||
except ValueError as e: | ||
raise ValidationError(PRICE_VALID_ERROR) from e | ||
return price |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These validators belong with Snapshot
model, definitely not here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Snapshot model ? really?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think you meant sponsorship
model !!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right.
Summary by CodeRabbit
WalkthroughThis pull request introduces a new function to synchronize GitHub issues, enhances the sponsorship command for Slack to dynamically create sponsorship records, and adds a comprehensive Sponsorship model within a newly created Nest app. It includes the necessary Django configurations, admin settings, and database migrations, as well as test cases to validate the model’s behavior and update functionality. Additionally, the GitHub client integration and command validations have been implemented to ensure proper data handling and error reporting. Changes
Assessment against linked issues
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Nitpick comments (12)
backend/apps/nest/migrations/0001_initial.py (1)
30-30
: Consider using DecimalField for monetary values.Using FloatField for price_usd could lead to floating-point precision issues, which can be problematic for financial calculations. DecimalField is generally recommended for monetary values.
- ("price_usd", models.FloatField()), + ("price_usd", models.DecimalField(max_digits=10, decimal_places=2)),backend/apps/github/common.py (2)
4-5
: Enhance error handling when no GitHub token is found.
Currently, the code loadsGITHUB_TOKEN
from the environment but never checks if it isNone
. Consider raising a clear error or providing a fallback to avoid runtime exceptions if the environment variable is not set.Also applies to: 7-8
20-22
: Remove or leverage the unusedFETCH_ISSUE_ERROR
constant.
FETCH_ISSUE_ERROR
is defined but not referenced anywhere. Eliminating unused constants or incorporating them into relevant exception handling will help keep the codebase tidy.backend/apps/slack/commands/sponsor.py (3)
35-37
: Consider stricter command formatting checks.
validate_command_format
only checks the argument count. A more robust validation (e.g., verifying placeholders like<issue_link>
) might improve user experience and reduce errors.
39-100
: Refine logging messages for different exception types.
BothValidationError
and generic exceptions log the same "Validation error" message. Using distinct error messages helps debugging and clarifies the root cause.-except ValidationError as e: - logger.exception("Validation error") +except ValidationError as e: + logger.exception("Sponsor command validation error") -except Exception: - logger.exception("Validation error") +except Exception: + logger.exception("Generic error processing sponsor command")
130-149
: Add a hyperlink to the sponsored issue.
Including a direct link to the GitHub issue in the Slack confirmation message can enhance usability.f"🎉 *Sponsorship created successfully!* 🎉{NL}" f"*Issue:* {sponsorship.issue.title}{NL}" + f"<{sponsorship.issue.url}|View Issue on GitHub>{NL}"
backend/tests/nest/model/sponsorship_test.py (2)
12-77
: Expand test coverage to include edge cases.
The current tests cover typical use cases ofupdate_data
. Consider adding scenarios for negative prices, invalid deadlines, or missing Slack user IDs to ensure robust handling of all potential inputs.
78-105
: Consider testing invalid backgrounds when creating Sponsorships.
You might add a test ensuring that creating a Sponsorship with an invalidissue
or malformed data raises the correct error, strengthening confidence in model validations.backend/apps/nest/models/sponsorship.py (4)
18-19
: Remove the extra blank line for PEP 8 compliance.According to PEP 8, there should only be one blank line between top-level definitions, not two.
from apps.nest.constants import ( DEADLINE_FORMAT_ERROR, DEADLINE_FUTURE_ERROR, ISSUE_LINK_ERROR, PRICE_POSITIVE_ERROR, PRICE_VALID_ERROR, ) - class Sponsorship(BulkSaveModel, TimestampedModel):
28-28
: Use DecimalField instead of FloatField for monetary values.For storing monetary values like prices,
DecimalField
is recommended overFloatField
to avoid floating-point precision issues that could lead to rounding errors.- price_usd = models.FloatField() + price_usd = models.DecimalField(max_digits=10, decimal_places=2)This change would ensure more precise handling of monetary values, which is important for financial data.
73-79
: Enhance GitHub issue link validation.The current validation only checks if the URL starts with a specific prefix. Consider implementing a more thorough validation to ensure the link follows the expected format for GitHub issues.
@staticmethod def validate_github_issue_link(issue_link): """Validate that the issue link belongs to a valid OWASP-related repository.""" - if not issue_link.startswith("https://github.com/OWASP"): + import re + # Pattern to match GitHub issue URLs in OWASP repositories + pattern = r"^https://github\.com/OWASP/[\w.-]+/issues/\d+$" + if not re.match(pattern, issue_link): raise ValidationError(ISSUE_LINK_ERROR) return issue_link
80-89
: Validate price with currency precision.Since this is a monetary value, consider validating that the price has appropriate precision for currency (typically two decimal places).
@staticmethod def validate_price(price): """Validate that the price is a positive float value.""" try: price = float(price) if price <= 0: raise ValidationError(PRICE_POSITIVE_ERROR) + # Ensure the price has at most 2 decimal places + if price != round(price, 2): + raise ValidationError("Price must have at most 2 decimal places") except ValueError as e: raise ValidationError(PRICE_VALID_ERROR) from e return price
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
backend/apps/github/common.py
(2 hunks)backend/apps/github/models/issue.py
(1 hunks)backend/apps/nest/admin.py
(1 hunks)backend/apps/nest/apps.py
(1 hunks)backend/apps/nest/constants.py
(1 hunks)backend/apps/nest/migrations/0001_initial.py
(1 hunks)backend/apps/nest/models/sponsorship.py
(1 hunks)backend/apps/slack/commands/sponsor.py
(2 hunks)backend/settings/base.py
(1 hunks)backend/tests/nest/model/sponsorship_test.py
(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- backend/apps/nest/apps.py
- backend/apps/nest/constants.py
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: CodeQL (python)
- GitHub Check: Run backend tests
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run frontend unit tests
🔇 Additional comments (7)
backend/settings/base.py (1)
48-48
: LGTM: Registered the Nest app with Django.The addition of "apps.nest" to the LOCAL_APPS tuple correctly registers the new Nest application with Django's configuration system, making it available throughout the project.
backend/apps/github/models/issue.py (1)
154-154
: Good defensive programming change.Adding the
self.project and
check prevents potential AttributeError ifself.project
is None, making the code more robust.backend/apps/nest/admin.py (1)
1-11
: Admin configuration for Sponsorship model looks good.The admin configuration appropriately displays key fields (issue, price_usd, slack_user_id) and enables searching by issue title and slack user ID, which will be helpful for administrators.
backend/apps/nest/migrations/0001_initial.py (1)
41-44
: LGTM: Good model configuration.The model configuration is appropriately set with a clear verbose name and a well-structured database table name.
backend/apps/github/common.py (1)
28-32
: Verify data freshness when an existing Issue object is found.
The function immediately returns after finding an existing Issue, meaning no updated data is fetched from GitHub. If continuous synchronization is expected, consider updating the Issue even if it already exists.backend/apps/slack/commands/sponsor.py (1)
17-22
: Good use of constants for command parameters.
Defining constants (COMMAND_FORMAT_ERROR
,DATE_INDEX
, etc.) clarifies the code and helps avoid magic numbers.backend/tests/nest/model/sponsorship_test.py (1)
106-137
: Comprehensive coverage for deadline usage.
These tests thoroughly validate configuring a deadline at creation time. Good work ensuring that thedeadline_at
field is set correctly.
("nest_updated_at", models.DateTimeField(auto_now=True)), | ||
("deadline_at", models.DateTimeField(blank=True, null=True)), | ||
("price_usd", models.FloatField()), | ||
("slack_user_id", models.CharField(max_length=100)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Verification agent
🧩 Analysis chain
Validate the slack_user_id field.
Consider adding validation for the slack_user_id field format to ensure it matches Slack's user ID pattern.
🏁 Script executed:
#!/bin/bash
# Check if the Sponsorship model has validation for slack_user_id in the model definition
# Find the Sponsorship model definition
fd -e py -t f "sponsorship.py" | xargs grep -A 20 "class Sponsorship"
Length of output: 686
Action Required: Implement Slack User ID Validation
The Sponsorship model currently defines the slack_user_id
field without any validation (see sponsorship.py
), so the field does not enforce Slack’s expected user ID format. To avoid potential data quality issues, please add appropriate validation (for example, using Django's RegexValidator
with a pattern matching Slack user IDs, such as one starting with "U" followed by alphanumeric characters). Note that changes should be made to the model definition; migration files will be updated accordingly in subsequent auto-generated migrations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Nitpick comments (2)
backend/apps/github/common.py (2)
28-29
: Improve function documentation with parameter and return value details.The function documentation should include information about its parameter and return value to help developers understand how to use it properly.
def sync_issue(issue_link): - """Sync GitHub issue data.""" + """ + Sync GitHub issue data. + + Args: + issue_link (str): URL of the GitHub issue to synchronize + + Returns: + Issue: The synchronized Issue object from the database + + Raises: + ValidationError: If the issue link format is invalid or the issue cannot be fetched + """
43-45
: Validate issue_number as a positive integer.The code converts
path_parts[3]
to an integer but doesn't verify that it's a positive number, which could lead to incorrect API calls if the value is not a valid issue number.issue_number = int(path_parts[3]) + if issue_number <= 0: + raise ValidationError("Invalid issue number in GitHub issue link.") owner = path_parts[0] repo_name = path_parts[1]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/apps/github/common.py
(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: CodeQL (python)
- GitHub Check: Run frontend unit tests
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run backend tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/apps/github/common.py
(2 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
backend/apps/github/common.py
20-20: SyntaxError: Unexpected indentation
46-46: SyntaxError: Expected a statement
93-93: SyntaxError: Expected except
or finally
after try
block
🪛 GitHub Actions: Run CI/CD
backend/apps/github/common.py
[error] 20-20: SyntaxError: Unexpected indentation
[error] 46-46: SyntaxError: Expected a statement
[error] 93-93: SyntaxError: Expected except
or finally
after try
block
[error] 20-20: Failed to parse: IndentationError: unexpected indent
[error] 20-20: IndentationError: unexpected indent in check_builtin_literals
🔇 Additional comments (1)
backend/apps/github/common.py (1)
29-30
: Define required constants before usage.The variables
MIN_PARTS_LENGTH
,INVALID_ISSUE_LINK_FORMAT
, andFETCH_ISSUE_ERROR
are used for validation and error handling but haven’t been defined in this file (or imported). Add their definitions or imports before using them.Also applies to: 43-43
backend/apps/github/common.py
Outdated
@@ -13,9 +17,91 @@ | |||
from apps.github.models.user import User | |||
from apps.github.utils import check_owasp_site_repository | |||
|
|||
def sync_issue(issue_link): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix indentation and remove duplicated code to resolve syntax errors.
• Line 20 has unexpected indentation before def sync_issue(issue_link):
.
• An empty line at 45 followed by a line at 46 seems to disrupt the flow of the try block above.
• Also, there's a second sync_issue
definition at lines 49–103, which duplicates the logic introduced at lines 20–44.
Combine your logic into a single sync_issue
function and ensure proper indentation:
- def sync_issue(issue_link):
- """Sync GitHub issue data."""
- try:
- return Issue.objects.get(url=issue_link)
- except Issue.DoesNotExist:
- pass
- ...
- except Exception as error:
- raise ValidationError(FETCH_ISSUE_ERROR.format(error=error))
-
- (line 45 is empty)
-
logger = logging.getLogger(__name__)
# Duplicate definition begins at line 49...
Also applies to: 46-46
🧰 Tools
🪛 Ruff (0.8.2)
20-20: SyntaxError: Unexpected indentation
🪛 GitHub Actions: Run CI/CD
[error] 20-20: SyntaxError: Unexpected indentation
[error] 20-20: Failed to parse: IndentationError: unexpected indent
[error] 20-20: IndentationError: unexpected indent in check_builtin_literals
backend/apps/github/common.py
Outdated
try: | ||
repository = Repository.objects.get(node_id=gh_repo.id) | ||
except Repository.DoesNotExist: | ||
try: | ||
repo_owner = User.objects.get(login=gh_repo.owner.login) | ||
except User.DoesNotExist: | ||
repo_owner = User.update_data(gh_repo.owner) | ||
try: | ||
if gh_repo.organization: | ||
try: | ||
organization = Organization.objects.get(node_id=gh_repo.organization.id) | ||
except Organization.DoesNotExist: | ||
organization = Organization.update_data(gh_repo.organization) | ||
else: | ||
organization = None | ||
|
||
repository = Repository.update_data( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Close the active try block to prevent syntax errors.
The try:
on line 77 never ends with a corresponding except
or finally
. This leads to a parse error at line 93. Include an except
or finally
block to properly close the try:
scope. For example:
try:
repository = Repository.objects.get(node_id=gh_repo.id)
except Repository.DoesNotExist:
...
finally:
- repository = Repository.update_data(
+ # Or handle the finalization logic here as needed
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.8.2)
93-93: SyntaxError: Expected except
or finally
after try
block
🪛 GitHub Actions: Run CI/CD
[error] 93-93: SyntaxError: Expected except
or finally
after try
block
7937902
to
00da50e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (3)
backend/apps/github/common.py (3)
4-8
: Add a docstring for newly added imports.
These imports are correct and necessary, but adding a concise docstring or comment to clarify usage (e.g., “Imports required for parsing GitHub issue links and validating environment variables”) can improve maintainability.
28-34
: Consider adding docstrings or direct reference to relevant models/tests.
While the logic for retrieving a pre-existingIssue
object is straightforward, it would be helpful to include a brief docstring or in-code reference to the Data Models or test cases validating this retrieval logic.
58-79
: Potential large calls to GitHub insync_issue
.
Fetching commits, contributors, and languages can be time-consuming for large repositories. While acceptable for smaller projects, you could consider an async approach, background job queue, or pagination to optimize performance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/apps/github/common.py
(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: CodeQL (python)
- GitHub Check: Run frontend e2e tests
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: Run backend tests
- GitHub Check: Run frontend unit tests
🔇 Additional comments (6)
backend/apps/github/common.py (6)
20-22
: Constants defined properly.
Defining error messages and minimum path length as constants fosters maintainability. This clear separation of concerns is good practice.
35-39
: Gracefully handle the integer conversion for the issue number.
Ifpath_parts[3]
is not an integer, this will raise aValueError
. Consider adding a try/except around theint()
call to provide a more explicit error message.Here's an example snippet for robust error handling:
- issue_number = int(path_parts[3]) + try: + issue_number = int(path_parts[3]) + except ValueError: + raise ValidationError("Issue number is not a valid integer.")Also applies to: 43-45
40-41
: Check for GitHub token presence before usage.
If the token is missing, the GitHub client instantiation may fail at runtime. This comment mirrors a previous suggestion.github_token = os.getenv("GITHUB_TOKEN") +if not github_token: + raise ValidationError("GitHub token not found in environment variables.") github_client = Github(github_token)
47-51
: Excellent use ofFETCH_ISSUE_ERROR
constant.
Utilizing a try/except block with the custom error message is a clean way to capture and rethrow GitHub API exceptions.
53-57
: Author retrieval logic looks solid.
Fetching or creating aUser
object tied to thegh_issue.user
is appropriate. This design is consistent with best practices for user synchronization.
65-69
: Handle missing organization objects.
Ifgh_repo.organization
isNone
, accessinggh_repo.organization.id
will cause anAttributeError
. Checking for the organization’s presence explicitly ensures more robust handling. This also mirrors a past suggestion.try: + if not gh_repo.organization: + organization = None + else: organization = Organization.objects.get(node_id=gh_repo.organization.id) except Organization.DoesNotExist: organization = Organization.update_data(gh_repo.organization)
if len(parts) > TIME_INDEX: | ||
deadline_str += " " + parts[TIME_INDEX] | ||
|
||
Sponsorship.validate_github_issue_link(issue_link) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's interesting i will use validation on saving.
verbose_name_plural = "Sponsorships" | ||
|
||
deadline_at = models.DateTimeField(null=True, blank=True) | ||
price_usd = models.FloatField() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's rename price_usd
to amount
and add currency
(EUR, USD with USD default)
backend/apps/github/common.py
Outdated
if len(path_parts) < MIN_PARTS_LENGTH or path_parts[2] != "issues": | ||
raise ValidationError(INVALID_ISSUE_LINK_FORMAT) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The validation should happen upon saving the model instance. This function should accept a clean link w/o worrying about validation errors.
backend/apps/github/models/issue.py
Outdated
@@ -151,7 +151,7 @@ def generate_summary(self, open_ai=None, max_tokens=500): | |||
|
|||
prompt = ( | |||
Prompt.get_github_issue_documentation_project_summary() | |||
if self.project.is_documentation_type | |||
if self.project and self.project.is_documentation_type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also should be a part of validation on an upper level. If issue doesn't have a project it probably shouldn't be considered for sponsorship.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when I tried to delete the nest from the local database, it couldn't generate the summary. That's result is that issue was not created. This might be because I don't have the credentials for OpenAI.
could you look into it and tell how can we fix that ?
backend/apps/nest/constants.py
Outdated
ISSUES_INDEX = 5 | ||
GITHUB_COM_INDEX = 2 | ||
MIN_PARTS_LENGTH = 4 | ||
|
||
DEADLINE_FORMAT_ERROR = "Invalid deadline format. Use YYYY-MM-DD or YYYY-MM-DD HH:MM." | ||
DEADLINE_FUTURE_ERROR = "Deadline must be in the future." | ||
FETCH_ISSUE_ERROR = "Failed to fetch issue from GitHub: {error}" | ||
INVALID_ISSUE_LINK_FORMAT = "Invalid GitHub issue link format." | ||
ISSUE_LINK_ERROR = "Issue link must belong to an OWASP repository." | ||
PRICE_POSITIVE_ERROR = "Price must be a positive value." | ||
PRICE_VALID_ERROR = "Price must be a valid number." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Narrow down the scope -- add to a separate constants only something that you need in multiple modules.
"""Validate that the deadline is in a valid datetime format.""" | ||
try: | ||
# Try parsing the deadline in YYYY-MM-DD format | ||
deadline = datetime.strptime(deadline_str, "%Y-%m-%d").replace( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using python-dateutil parser for better date recognition
@abhayymishraa any updates on this? |
yes , i'm on it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
♻️ Duplicate comments (2)
backend/apps/github/common.py (2)
40-41
: 🛠️ Refactor suggestionCheck for missing GitHub token.
When the environment variable
GITHUB_TOKEN
is missing, the code instantiates aGithub
client withNone
. You can surface a more user-friendly error by verifying thatgithub_token
is present:github_token = os.getenv("GITHUB_TOKEN") +if not github_token: + raise ValidationError("GitHub token not found in environment variables.") github_client = Github(github_token)
58-69
:⚠️ Potential issueHandle the case when
gh_repo.organization
isNone
.Some repositories may not belong to an organization. Accessing
gh_repo.organization.id
or passinggh_repo.organization
toOrganization.update_data()
might raise anAttributeError
. Check ifgh_repo.organization
is notNone
before attempting to retrieve or update the organization:try: + if gh_repo.organization is None: + organization = None + else: + organization = Organization.objects.get(node_id=gh_repo.organization.id) except Organization.DoesNotExist: organization = Organization.update_data(gh_repo.organization)
🧹 Nitpick comments (6)
backend/apps/nest/admin.py (1)
6-11
: Consider adding admin filters.Currently, the
SponsorshipAdmin
class only specifieslist_display
andsearch_fields
. Adding filters likelist_filter
(e.g., forcurrency
ordeadline_at
) could improve usability for administrators who need to filter sponsorship records quickly.backend/apps/nest/models/sponsorship.py (1)
33-39
: Consider enforcing uniqueness for sponsorships per user and issue.If the intention is to prevent multiple sponsorships from the same Slack user for the same issue, add a unique constraint or
unique_together = ("issue", "slack_user_id")
in the model’sMeta
. This protects data integrity by preventing repeated or conflicting records.backend/apps/slack/commands/sponsor.py (2)
41-66
: Return an error message to the user instead of silently returning.When the command format is incorrect or the GitHub issue link format is invalid, the current code logs an error and simply returns. For better UX, provide an ephemeral Slack response or usage instructions so that users are aware of the error and know how to fix it.
153-173
: Consider modularizing complex Slack commands.As the
/sponsor
command grows, extracting sub-commands or smaller helpers can improve maintainability and readability. For instance, handle parsing, validation, and creation in separate functions or modules to reduce complexity in the main command flow.backend/apps/github/common.py (2)
30-35
: Consider adding test coverage forsync_issue
.This function is central to synchronizing GitHub issues. To ensure reliability and prevent regressions, add corresponding unit tests or integration tests if none currently exist.
79-81
: Uselogger.error
instead oflogger.exception
if no exception is raised.At this point, there’s no caught exception to log with
logger.exception()
. Consider usinglogger.error()
orlogger.warning()
to convey that the repository has no associated project, preventing a misleading traceback.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/apps/github/common.py
(2 hunks)backend/apps/nest/admin.py
(1 hunks)backend/apps/nest/migrations/0001_initial.py
(1 hunks)backend/apps/nest/models/sponsorship.py
(1 hunks)backend/apps/slack/commands/sponsor.py
(2 hunks)backend/tests/nest/model/sponsorship_test.py
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/tests/nest/model/sponsorship_test.py
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: CodeQL (python)
- GitHub Check: Run frontend unit tests
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run backend tests
🔇 Additional comments (2)
backend/apps/nest/migrations/0001_initial.py (1)
39-39
: Add Slack user ID validation.The
slack_user_id
field is introduced withmodels.CharField(max_length=100)
. A suitable validation approach (regex or custom validator) ensures that the field matches Slack’s expected user ID format (e.g., starts with "U" followed by alphanumeric characters).backend/apps/github/common.py (1)
20-24
: Constants look well-defined.Defining these error messages and constants up front improves readability and maintainability.
Hey @arkid15r , I've completed the requested changes, but it's not fully refined yet. There's still room for improvement, especially in handling validation errors more effectively. It works fine, but we can enhance it further. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
♻️ Duplicate comments (2)
backend/apps/github/common.py (2)
43-44
:⚠️ Potential issueCheck for GitHub token existence before use.
The code doesn't verify if the GitHub token is actually present in the environment variables. This could lead to authentication errors when making API calls.
github_token = os.getenv("GITHUB_TOKEN") +if not github_token: + raise ValidationError("GitHub token not found in environment variables.") github_client = Github(github_token)
68-71
:⚠️ Potential issueHandle the case where a repository doesn't have an organization.
The code tries to access
gh_repo.organization.id
without checking if the repository actually has an organization. This could raise an AttributeError if the repository doesn't belong to an organization.try: - organization = Organization.objects.get(node_id=gh_repo.organization.id) -except Organization.DoesNotExist: - organization = Organization.update_data(gh_repo.organization) + if gh_repo.organization: + try: + organization = Organization.objects.get(node_id=gh_repo.organization.id) + except Organization.DoesNotExist: + organization = Organization.update_data(gh_repo.organization) + else: + organization = None
🧹 Nitpick comments (1)
backend/apps/github/common.py (1)
33-86
: Consider adding docstring parameters and return type annotations.The
sync_issue
function would benefit from more detailed documentation, including parameter types and return value information. This would make the function more maintainable and easier to understand.def sync_issue(issue_link): - """Sync GitHub issue data.""" + """ + Sync GitHub issue data from the provided issue link. + + Args: + issue_link (str): URL to the GitHub issue + + Returns: + Issue: The synchronized Issue object, or None if validation fails + + Raises: + ValidationError: If the issue link format is invalid or the issue cannot be fetched + """
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/apps/github/common.py
(2 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
backend/apps/github/common.py (5)
backend/apps/github/models/issue.py (2) (2)
Issue
(16:165)update_data
(153:165)backend/apps/github/models/repository.py (4) (4)
url
(158:160)update_data
(266:293)Repository
(22:293)project
(135:137)backend/apps/github/models/user.py (2) (2)
User
(12:79)update_data
(67:79)backend/apps/github/models/repository_contributor.py (1) (1)
update_data
(61:78)backend/apps/github/models/organization.py (2) (2)
update_data
(57:69)Organization
(12:69)
if not repository.project: | ||
logger.exception(VALID_PROJECT_ERROR) | ||
return None | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Improve project validation error handling.
The code checks if the repository has a project, but only logs an error and returns None rather than raising the ValidationError with the message defined in the VALID_PROJECT_ERROR
constant. This inconsistency between error handling approaches could make debugging harder.
if not repository.project:
- logger.exception(VALID_PROJECT_ERROR)
- return None
+ logger.error(VALID_PROJECT_ERROR)
+ raise ValidationError(VALID_PROJECT_ERROR)
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
if not repository.project: | |
logger.exception(VALID_PROJECT_ERROR) | |
return None | |
if not repository.project: | |
logger.error(VALID_PROJECT_ERROR) | |
raise ValidationError(VALID_PROJECT_ERROR) |
parsed_url = urlparse(issue_link) | ||
path_parts = parsed_url.path.strip("/").split("/") | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Verification agent
🧩 Analysis chain
Validate URL path before extracting issue components.
The code parses the issue link but doesn't validate if it's a proper GitHub URL or if path_parts
contains enough elements before accessing them. This can lead to IndexError exceptions when accessing path_parts[3]
on line 46.
parsed_url = urlparse(issue_link)
path_parts = parsed_url.path.strip("/").split("/")
+
+if len(path_parts) < MIN_PARTS_LENGTH or path_parts[2] != "issues":
+ raise ValidationError(INVALID_ISSUE_LINK_FORMAT)
Note: The constant INVALID_ISSUE_LINK_FORMAT
appears to be missing from the file and needs to be defined.
I'll start by running a shell script to search for the definitions of the constants MIN_PARTS_LENGTH
and INVALID_ISSUE_LINK_FORMAT
across the repository to verify if they exist. This will help us determine if the proposed validation changes are complete and if the missing constant needs to be defined.
🏁 Script executed:
#!/bin/bash
echo "Searching for MIN_PARTS_LENGTH defined in the repository..."
rg "MIN_PARTS_LENGTH" .
echo "Searching for INVALID_ISSUE_LINK_FORMAT defined in the repository..."
rg "INVALID_ISSUE_LINK_FORMAT" .
Length of output: 760
Critical: Validate GitHub Issue URL and Define Missing Constant
The updated validation logic correctly checks if the URL path contains enough parts and whether the issue keyword is present. However, while MIN_PARTS_LENGTH
is defined in this file, the constant INVALID_ISSUE_LINK_FORMAT
is missing and should be defined (or imported) to avoid runtime errors.
- Action Required:
-
Define
INVALID_ISSUE_LINK_FORMAT
inbackend/apps/github/common.py
, for example:INVALID_ISSUE_LINK_FORMAT = "Invalid GitHub issue link format."
-
Ensure that the newly added validation snippet is integrated as follows:
parsed_url = urlparse(issue_link) path_parts = parsed_url.path.strip("/").split("/") + +if len(path_parts) < MIN_PARTS_LENGTH or path_parts[2] != "issues": + raise ValidationError(INVALID_ISSUE_LINK_FORMAT)
-
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
♻️ Duplicate comments (1)
backend/apps/github/common.py (1)
40-48
:⚠️ Potential issueAdd checks for URL format and missing GitHub token.
- Currently there's no validation to ensure
path_parts
has enough elements, which could cause anIndexError
if the URL format is off.- Checking if
GITHUB_TOKEN
is set helps avoid authentication failures.Sample fix:
parsed_url = urlparse(issue_link) path_parts = parsed_url.path.strip("/").split("/") +if len(path_parts) < MIN_PARTS_LENGTH or path_parts[2] != "issues": + raise ValidationError("Invalid GitHub issue link format.") github_token = os.getenv("GITHUB_TOKEN") +if not github_token: + raise ValidationError("GitHub token not found in environment variables.") github_client = Github(github_token) issue_number = int(path_parts[3]) owner = path_parts[0] repo_name = path_parts[1]
🧹 Nitpick comments (1)
backend/apps/github/common.py (1)
82-85
: Uselogger.error
instead oflogger.exception
.
logger.exception
typically prints a stack trace, which is more suited to error blocks. If no exception is raised, considerlogger.error
to clearly log the invalid project scenario.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/apps/github/common.py
(2 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
backend/apps/github/common.py (5)
backend/apps/github/models/issue.py (2)
Issue
(16-165)update_data
(153-165)backend/apps/github/models/repository.py (4)
url
(158-160)update_data
(266-293)Repository
(22-293)project
(135-137)backend/apps/github/models/user.py (2)
User
(12-79)update_data
(67-79)backend/apps/github/models/organization.py (2)
update_data
(57-69)Organization
(12-69)backend/apps/github/models/repository_contributor.py (1)
update_data
(61-78)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: CodeQL (python)
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: Run backend tests
- GitHub Check: Run frontend unit tests
- GitHub Check: Run frontend e2e tests
🔇 Additional comments (6)
backend/apps/github/common.py (6)
4-4
: Imports look good.
These imports for environment variables, URL parsing, Django exceptions, and the GitHub library are appropriate and straightforward.Also applies to: 6-6, 8-8, 10-10
33-38
: Retrieval shortcut looks fine.
Early return of an existingIssue
by URL fosters code reuse. No concerns here.
50-54
: Unified error handling is appropriate.
Wrapping GitHub fetch logic in a try/except block and raising a singleValidationError
ensures consistent error messaging.
56-60
: User creation fallback.
Falling back toUser.update_data
when the user does not exist locally is a sensible approach that helps keep user data in sync.
61-80
: Handle the case whengh_repo.organization
is None.
Line 69 accessesgh_repo.organization.id
without confirming thatgh_repo.organization
exists. This can cause anAttributeError
if the repository lacks an organization.
86-86
: Final return is logically consistent.
Returning the updated or newly createdIssue
matches the function’s core purpose of syncing issue data.
FETCH_ISSUE_ERROR = "Failed to fetch issue from GitHub: {error}" | ||
MIN_PARTS_LENGTH = 4 | ||
VALID_PROJECT_ERROR = ( | ||
"Issue does not have a valid project and cannot be considered for sponsorship." | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider adding a validation constant for malformed issue links.
While these constants help standardize error messages, there's no definition for something like INVALID_ISSUE_LINK_FORMAT
if you plan to validate the link structure. Additionally, if MIN_PARTS_LENGTH
remains unused, consider removing or referencing it when validating path_parts
.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
♻️ Duplicate comments (4)
backend/apps/github/common.py (4)
43-44
:⚠️ Potential issueCheck for GitHub token existence before use.
The code doesn't check if the GitHub token is actually present in the environment variables. If the token is missing, it could lead to authentication errors when making API calls.
github_token = os.getenv("GITHUB_TOKEN") +if not github_token: + raise ValidationError("GitHub token not found in environment variables.") github_client = Github(github_token)
68-71
:⚠️ Potential issueHandle the case where a repository doesn't have an organization.
The code tries to access
gh_repo.organization.id
without checking if the repository actually has an organization. This could raise an AttributeError if the repository doesn't belong to an organization.try: - organization = Organization.objects.get(node_id=gh_repo.organization.id) - except Organization.DoesNotExist: - organization = Organization.update_data(gh_repo.organization) + if gh_repo.organization: + try: + organization = Organization.objects.get(node_id=gh_repo.organization.id) + except Organization.DoesNotExist: + organization = Organization.update_data(gh_repo.organization) + else: + organization = None
82-85
:⚠️ Potential issueImprove project validation error handling.
The code checks if the repository has a project, but only logs an error and returns None rather than raising the ValidationError with the message defined in the
VALID_PROJECT_ERROR
constant. This inconsistency between error handling approaches could make debugging harder.if not repository.project: - logger.exception(VALID_PROJECT_ERROR) - return None + logger.error(VALID_PROJECT_ERROR) + raise ValidationError(VALID_PROJECT_ERROR)
40-42
:⚠️ Potential issueValidate URL path format before accessing components.
The code parses the issue link but doesn't validate if it's a proper GitHub URL or if
path_parts
contains enough elements before accessing them. This can lead to IndexError exceptions when accessingpath_parts[3]
on line 46.parsed_url = urlparse(issue_link) path_parts = parsed_url.path.strip("/").split("/") +if len(path_parts) < MIN_PARTS_LENGTH or path_parts[2] != "issues": + raise ValidationError(INVALID_ISSUE_LINK_FORMAT)
🧹 Nitpick comments (5)
backend/apps/slack/commands/sponsor.py (4)
39-41
: Move imports to the top of the file.It's better practice to place imports at the top of the file rather than inside functions. This improves readability and prevents potential issues with circular imports or repeated import overhead.
"""Slack bot sponsors command.""" import logging from urllib.parse import urlparse +from apps.github.common import sync_issue +from apps.nest.models.sponsorship import Sponsorship from dateutil.parser import parse as date_parse from django.conf import settings from django.core.exceptions import ValidationError from django.db import transaction from django.utils import timezone from apps.common.constants import NL from apps.slack.apps import SlackConfig from apps.slack.utils import get_text logger = logging.getLogger(__name__) COMMAND = "/sponsor" COMMAND_FORMAT_ERROR = ( "Invalid command format. Usage: `/sponsor task add <issue_link> <amount>[EUR|USD] [deadline]`" ) DEADLINE_FORMAT_ERROR = "Invalid deadline format. Use YYYY-MM-DD or YYYY-MM-DD HH:MM." INVALID_ISSUE_LINK_FORMAT = "Invalid GitHub issue link format." DATE_INDEX = 4 MIN_PARTS_LENGTH = 4 TIME_INDEX = 5 def sponsor_handler(ack, command, client): """Handle the Slack /sponsor command. Args: ack (function): Acknowledge the Slack command request. command (dict): The Slack command payload. client (slack_sdk.WebClient): The Slack WebClient instance for API calls. """ - from apps.github.common import sync_issue - from apps.nest.models.sponsorship import Sponsorship -🧰 Tools
🪛 Ruff (0.8.2)
41-41: Blank line contains whitespace
Remove whitespace from blank line
(W293)
68-73
: Validate GitHub issue link earlier in the function.The issue link validation should happen before attempting to extract currency and price, as it's a more fundamental requirement. This would prevent unnecessary processing if the link is invalid.
- parsed_url = urlparse(issue_link) - path_parts = parsed_url.path.strip("/").split("/") - if len(path_parts) < MIN_PARTS_LENGTH or path_parts[2] != "issues": - logger.error("Invalid GitHub issue link format") - return # Move this code block earlier, right after issue_link = parts[2]
104-115
: Add more specific error messages for validation errors.The current implementation catches ValidationError but doesn't provide detailed information about the specific validation that failed. Adding more context would help users understand what went wrong and how to fix it.
blocks = get_sponsorship_blocks(sponsorship) except ValidationError as e: logger.exception("Validation error") blocks = [ { "type": "section", "text": { "type": "mrkdwn", - "text": f"❌ *Error:* {e!s}{NL}", + "text": f"❌ *Validation Error:* {e!s}{NL}Please check your input and try again.", }, } ]
116-127
: Use a more specific log message for general exceptions.The current code logs "Validation error" for both validation errors and general exceptions, which can be confusing when debugging. It's better to use a more specific message for general exceptions.
except Exception: - logger.exception("Validation error") + logger.exception("Unexpected error in sponsor command") blocks = [ { "type": "section", "text": { "type": "mrkdwn", "text": ( f"❌ *Error:* An error occurred while processing your request.{NL}" ), }, } ]backend/apps/github/common.py (1)
61-80
: Consider adding more detailed logging.For complex operations like repository syncing that involve multiple steps and potential failure points, it would be helpful to add more detailed debug logging. This would make troubleshooting easier in production environments.
try: repository = Repository.objects.get(node_id=gh_repo.id) + logger.debug("Found existing repository: %s", repository.name) except Repository.DoesNotExist: + logger.debug("Repository not found, creating new one: %s/%s", owner, repo_name) try: repo_owner = User.objects.get(login=gh_repo.owner.login) + logger.debug("Found existing repo owner: %s", repo_owner.login) except User.DoesNotExist: + logger.debug("Repo owner not found, creating new one: %s", gh_repo.owner.login) repo_owner = User.update_data(gh_repo.owner)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/apps/github/common.py
(2 hunks)backend/apps/slack/commands/sponsor.py
(3 hunks)backend/settings/base.py
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/settings/base.py
🧰 Additional context used
🧬 Code Definitions (1)
backend/apps/slack/commands/sponsor.py (2)
backend/apps/github/common.py (1)
sync_issue
(33-86)backend/apps/nest/models/sponsorship.py (3)
Sponsorship
(15-74)CurrencyType
(18-22)save
(61-64)
🪛 Ruff (0.8.2)
backend/apps/slack/commands/sponsor.py
41-41: Blank line contains whitespace
Remove whitespace from blank line
(W293)
🪛 GitHub Actions: Run CI/CD
backend/apps/slack/commands/sponsor.py
[error] 38-38: Ruff found 1 error (1 fixed, 0 remaining).
[warning] 1-1: incorrect-blank-line-before-class
(D203) and no-blank-line-before-class
(D211) are incompatible. Ignoring incorrect-blank-line-before-class
.
[warning] 1-1: multi-line-summary-first-line
(D212) and multi-line-summary-second-line
(D213) are incompatible. Ignoring multi-line-summary-second-line
.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (6)
backend/apps/slack/commands/sponsor.py (2)
160-179
: This implementation for sponsorship confirmation blocks looks good.The
get_sponsorship_blocks
function is well-structured and correctly handles different currency types and optional deadline formatting.
86-101
:✅ Verification successful
Consider using Sponsorship model's validation.
Since a previous comment mentioned using Django's
full_clean()
for validation, and the author agreed to "use validation on saving", ensure that theSponsorship
model is properly validating data when saving. Thesave()
method should callfull_clean()
to validate all fields.
🏁 Script executed:
#!/bin/bash # Check if Sponsorship model properly validates data before saving echo "Checking Sponsorship model's save method:" rg -A 5 "def save" backend/apps/nest/models/sponsorship.py echo "Checking if full_clean is called in save method:" rg "full_clean" backend/apps/nest/models/sponsorship.pyLength of output: 517
Sponsorship model validation is correctly implemented
The
Sponsorship
model'ssave()
method callsfull_clean()
as required, so the validation is functioning as expected. No additional changes are needed in this area.backend/apps/github/common.py (4)
33-38
: The issue retrieval implementation looks good.The function properly attempts to retrieve an existing issue from the database before proceeding to fetch it from GitHub.
50-54
: Good error handling for GitHub API calls.The code properly catches exceptions from GitHub API calls and raises a meaningful validation error with the original error message and context.
56-60
: Good handling of user data.The code properly checks if the author already exists in the database, and if not, creates a new user record with data from GitHub.
86-86
: The issue update implementation looks good.The function correctly returns the updated Issue object with the author and repository information.
currency = Sponsorship.CurrencyType.USD | ||
price = price_input | ||
|
||
if price_input.endswith("EUR"): | ||
currency = "EUR" | ||
price = price_input[:-3] | ||
elif price_input.endswith("USD"): | ||
currency = "USD" | ||
price = price_input[:-3] | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Validate price input as a numeric value.
The code extracts the currency type but doesn't validate if the price string can be converted to a number. If a user inputs non-numeric characters (e.g., "abc100EUR"), this will cause a runtime error when saving to the database.
currency = Sponsorship.CurrencyType.USD
price = price_input
if price_input.endswith("EUR"):
currency = "EUR"
price = price_input[:-3]
elif price_input.endswith("USD"):
currency = "USD"
price = price_input[:-3]
+ try:
+ float(price)
+ except ValueError:
+ logger.error("Invalid price format: %s", price_input)
+ blocks = [
+ {
+ "type": "section",
+ "text": {
+ "type": "mrkdwn",
+ "text": f"❌ *Error:* Price must be a numeric value.{NL}",
+ },
+ }
+ ]
+ conversation = client.conversations_open(users=command["user_id"])
+ client.chat_postMessage(
+ blocks=blocks,
+ channel=conversation["channel"]["id"],
+ text=get_text(blocks),
+ )
+ return
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
currency = Sponsorship.CurrencyType.USD | |
price = price_input | |
if price_input.endswith("EUR"): | |
currency = "EUR" | |
price = price_input[:-3] | |
elif price_input.endswith("USD"): | |
currency = "USD" | |
price = price_input[:-3] | |
currency = Sponsorship.CurrencyType.USD | |
price = price_input | |
if price_input.endswith("EUR"): | |
currency = "EUR" | |
price = price_input[:-3] | |
elif price_input.endswith("USD"): | |
currency = "USD" | |
price = price_input[:-3] | |
try: | |
float(price) | |
except ValueError: | |
logger.error("Invalid price format: %s", price_input) | |
blocks = [ | |
{ | |
"type": "section", | |
"text": { | |
"type": "mrkdwn", | |
"text": f"❌ *Error:* Price must be a numeric value.{NL}", | |
}, | |
} | |
] | |
conversation = client.conversations_open(users=command["user_id"]) | |
client.chat_postMessage( | |
blocks=blocks, | |
channel=conversation["channel"]["id"], | |
text=get_text(blocks), | |
) | |
return |
Resolves #916
Add the PR description here.
Screenshot/Preview
Results with command