Added AI processing to events #1066
Conversation
Summary by CodeRabbit
WalkthroughThis pull request integrates OWASP event enrichment across the system. On the backend, a new dependency and management command have been added, the Prompt model has been extended with OWASP-specific methods, and the Event model has been updated via migrations to include new geographical and summary fields (with a subsequent removal of some fields). The GraphQL API has also been extended to expose additional event details. On the frontend, tests and components have been adjusted to replace dynamic modal properties with a static description, update query fields, add modal functionality for event details, and make minor UI refinements, including updates to text content and styling. Changes
Suggested labels
Suggested reviewers
✨ 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (8)
frontend/__tests__/unit/pages/Modal.test.tsx (1)
29-42: LGTM: Added entityType property to defaultPropsThe addition of the
entityType: 'issue'property to the defaultProps object is correct and aligns with the test expectations. Line 89 checks for text that refers to "issue" in the modal.One suggestion to enhance test coverage would be to add test cases that verify the modal renders correctly with different entity types (e.g., 'event'). This would ensure that entity-specific text is displayed correctly for all supported entity types.
frontend/src/components/Modal.tsx (2)
26-26: Add default value for entityType prop.While the entityType prop works correctly, it's missing a default value which could cause runtime errors if undefined is passed.
- entityType, + entityType = 'issue',
62-64: Consider extracting string capitalization into a utility function.The string capitalization logic could be extracted into a reusable utility function for better maintainability and reuse across the codebase.
- <Text className="mb-2 text-xl font-semibold"> - {entityType[0].toUpperCase() + entityType.substring(1)} Summary - </Text> + <Text className="mb-2 text-xl font-semibold"> + {capitalizeFirstLetter(entityType)} Summary + </Text>You could define this utility function:
// In a utils file export const capitalizeFirstLetter = (string: string): string => { return string.charAt(0).toUpperCase() + string.slice(1); }backend/apps/owasp/management/commands/owasp_enrich_events.py (2)
34-35: Line exceeds recommended length.This line exceeds the recommended length of 99 characters. Consider breaking it into multiple lines for better readability.
- if not event.suggested_location and (prompt := Prompt.get_owasp_event_suggested_location()): + if not event.suggested_location and ( + prompt := Prompt.get_owasp_event_suggested_location() + ):🧰 Tools
🪛 Ruff (0.8.2)
34-34: Line too long (104 > 99)
(E501)
30-36: Add error handling for summary and location generation.While there's error handling for geo-location generation, there's none for summary and suggested location generation. Consider adding similar try-except blocks for these operations.
# Summary. if not event.summary and (prompt := Prompt.get_owasp_event_summary()): + try: event.generate_summary(prompt=prompt) + except Exception: + logger.exception( + "Could not generate summary for event", + extra={"url": event.url}, + ) # Suggested location. if not event.suggested_location and (prompt := Prompt.get_owasp_event_suggested_location()): + try: event.generate_suggested_location() + except Exception: + logger.exception( + "Could not generate suggested location for event", + extra={"url": event.url}, + )🧰 Tools
🪛 Ruff (0.8.2)
34-34: Line too long (104 > 99)
(E501)
frontend/src/pages/Home.tsx (1)
156-164: Modal implementation is well-structured but consider adding accessibility attributes.The Modal component is correctly implemented with the required props for displaying event details, but it could benefit from improved accessibility.
Consider adding accessibility attributes to the button, such as
aria-labelto clearly indicate it opens a modal:<button className="mb-2 w-full text-left text-lg font-semibold text-blue-500 hover:underline" onClick={() => setModalOpenIndex(index)} + aria-label={`Open details for ${event.name}`} > <h3 className="truncate">{event.name}</h3> </button>backend/apps/owasp/models/event.py (2)
172-182: Add error handling to the generate_geo_location method.The current implementation doesn't handle potential errors from the
get_location_coordinatesfunction.Add try-except blocks to handle potential geocoding errors:
def generate_geo_location(self): """Add latitude and longitude data.""" location = None if self.suggested_location and self.suggested_location != "None": - location = get_location_coordinates(self.suggested_location) + try: + location = get_location_coordinates(self.suggested_location) + except Exception: + # Log the error but continue to the fallback + pass if location is None: - location = get_location_coordinates(self.get_geo_string()) + try: + location = get_location_coordinates(self.get_geo_string()) + except Exception: + # Log the error + pass if location: self.latitude = location.latitude self.longitude = location.longitude
183-183: Method name doesn't match its documentation.The docstring says "Generate project summary" but the method is actually generating a suggested location for an event.
Update the docstring to accurately reflect the method's purpose:
def generate_suggested_location(self, open_ai=None, max_tokens=100): - """Generate project summary.""" + """Generate a suggested location for the event using OpenAI."""
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
backend/Makefile(1 hunks)backend/apps/core/models/prompt.py(1 hunks)backend/apps/owasp/graphql/nodes/event.py(1 hunks)backend/apps/owasp/management/commands/owasp_enrich_events.py(1 hunks)backend/apps/owasp/migrations/0023_event_latitude_event_longitude_and_more.py(1 hunks)backend/apps/owasp/migrations/0024_event_country_event_postal_code_event_region.py(1 hunks)backend/apps/owasp/models/event.py(3 hunks)frontend/__tests__/e2e/pages/Home.spec.ts(1 hunks)frontend/__tests__/unit/pages/Modal.test.tsx(1 hunks)frontend/src/api/queries/homeQueries.ts(1 hunks)frontend/src/components/Modal.tsx(2 hunks)frontend/src/pages/Contribute.tsx(1 hunks)frontend/src/pages/Home.tsx(3 hunks)frontend/src/types/event.ts(1 hunks)frontend/src/types/modal.ts(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
backend/apps/owasp/migrations/0024_event_country_event_postal_code_event_region.py
9-9: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
9-9: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
14-14: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
15-15: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
16-16: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
16-16: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
19-19: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
20-20: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
21-21: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
21-21: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
21-21: Line too long (102 > 99)
(E501)
24-24: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
25-25: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
26-26: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
26-26: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
backend/apps/owasp/management/commands/owasp_enrich_events.py
34-34: Line too long (104 > 99)
(E501)
backend/apps/owasp/migrations/0023_event_latitude_event_longitude_and_more.py
9-9: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
9-9: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
14-14: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
15-15: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
16-16: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
19-19: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
20-20: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
21-21: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
24-24: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
25-25: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
26-26: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
26-26: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
26-26: Line too long (110 > 99)
(E501)
29-29: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
30-30: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
31-31: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
31-31: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
🔇 Additional comments (16)
frontend/src/types/event.ts (1)
8-8: Looks good - Summary field addition is appropriateThe addition of an optional
summaryfield to theEventTypeinterface properly extends the event data structure to support the AI-generated summaries introduced in this PR.frontend/src/pages/Contribute.tsx (1)
67-67: Good implementation of new entityType propThe addition of the
entityType="issue"property to the Modal component in the Contribute page properly implements the interface change and correctly identifies the type of entity being displayed.backend/apps/owasp/graphql/nodes/event.py (1)
12-24: Added new fields to the EventNode properly.The additions to the
fieldstuple in theEventNodeMeta class look good. The new fields (country, postal_code, region, and summary) will now be exposed through the GraphQL API, allowing clients to query this additional event information.backend/apps/core/models/prompt.py (1)
74-83:❓ Verification inconclusive
Well-implemented static methods for event prompts.
The new static methods
get_owasp_event_suggested_location()andget_owasp_event_summary()follow the established pattern in the codebase. They both have clear docstrings and correctly use the existingget_text()method to retrieve prompts by key.One recommendation: verify that the prompt keys "owasp-event-suggested-location" and "owasp-event-summary" have been added to the database. If these keys don't exist, the methods will return None and log warnings when called.
🏁 Script executed:
#!/bin/bash # Check if the required prompt keys exist in the database echo "Checking for required prompt keys..." python3 -c " import django import os os.environ.setdefault('DJANGO_SETTINGS_MODULE', 'config.settings.local') django.setup() from apps.core.models.prompt import Prompt keys = ['owasp-event-suggested-location', 'owasp-event-summary'] for key in keys: exists = Prompt.objects.filter(key=key).exists() print(f'Prompt key \"{key}\" exists: {exists}') "Length of output: 589
Action: Verify Prompt Keys in the Database
The new static methods in
backend/apps/core/models/prompt.py(lines 74–83) are implemented correctly—they follow the established pattern with clear docstrings and proper usage ofPrompt.get_text(). However, please be aware that the verification script for checking the existence of the prompt keys"owasp-event-suggested-location"and"owasp-event-summary"encountered an environment error (ModuleNotFoundError: No module named 'django'). This error indicates that the verification was not executed in a Django-configured environment.Next Steps:
- Manual Verification Required: Please run the prompt key verification in an environment where Django is installed and configured (with the
DJANGO_SETTINGS_MODULEset appropriately).- Ensure Database Integrity: Confirm that the prompt keys exist in the database to avoid potential issues (i.e., methods returning
Noneand logging warnings).frontend/__tests__/e2e/pages/Home.spec.ts (1)
73-76: UI element role changed correctly from link to button.The test has been updated to reflect UI changes where event elements are now buttons instead of links. This change is consistent with implementing modal functionality for events rather than navigation.
backend/apps/owasp/migrations/0024_event_country_event_postal_code_event_region.py (1)
13-27: Migration looks good.This migration correctly adds the geographical fields (country, postal_code, region) to the Event model. Note that while postal_code allows blank values, country and region don't. This might be intentional, but if all geographical data is optional, consider making these consistent.
🧰 Tools
🪛 Ruff (0.8.2)
14-14: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
15-15: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
16-16: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
16-16: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
19-19: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
20-20: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
21-21: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
21-21: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
21-21: Line too long (102 > 99)
(E501)
24-24: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
25-25: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
26-26: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
26-26: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
backend/apps/owasp/migrations/0023_event_latitude_event_longitude_and_more.py (1)
13-32: Migration properly sets up geographical and descriptive fields.This migration correctly adds floating-point fields for coordinates and text fields for descriptive content. The use of null=True for coordinates is appropriate as empty strings wouldn't make sense for numeric data.
🧰 Tools
🪛 Ruff (0.8.2)
14-14: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
15-15: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
16-16: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
19-19: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
20-20: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
21-21: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
24-24: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
25-25: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
26-26: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
26-26: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
26-26: Line too long (110 > 99)
(E501)
29-29: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
30-30: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
31-31: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
31-31: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
backend/apps/owasp/management/commands/owasp_enrich_events.py (2)
17-18: Good use of optional argument.The command appropriately implements an optional offset argument, allowing for resuming processing from a specific point. This is particularly useful for large datasets or when the process needs to be restarted.
49-52:❓ Verification inconclusive
Consider including new geographical fields in bulk_save.
The bulk_save operation doesn't include the country, region, and postal_code fields that were added in migration 0024. If these fields are populated as part of generate_geo_location(), they should be included in the bulk_save fields list.
🏁 Script executed:
#!/bin/bash # Check if generate_geo_location() updates the country, region, and postal_code fields # First, look at the Event model definition to confirm field existence rg -A 10 "class Event" apps/owasp/models/event.py # Then examine the generate_geo_location method implementation rg -A 15 "def generate_geo_location" apps/owasp/models/event.pyLength of output: 251
Action Required: Please Verify Geographical Field Updates in bulk_save
The review comment noted that the new geographical fields (country, region, and postal_code) introduced in migration 0024 aren’t being saved via the bulk_save call. However, our initial verification attempt using the file path
apps/owasp/models/event.py
returned a “No such file or directory” error. This suggests that either the Event model (and its associated generate_geo_location function) is defined elsewhere, or the file path is outdated.Next steps for manual verification:
Confirm the Event model location: Search the repository for the definition of
class Event(to ensure you are inspecting the correct file.Verify generate_geo_location behavior: Locate the implementation of
generate_geo_location(using a repository-wide search for its definition) and verify if it populates thecountry,region, andpostal_codefields.Update bulk_save if applicable: If the new geographical fields are indeed being set by
generate_geo_location(), then update the bulk_save call to include them. For example:Event.bulk_save( all_events, fields=("latitude", "longitude", "suggested_location", "summary", "country", "region", "postal_code"), )Please verify these points manually to ensure the changes are consistent with the model definition and data-generation logic.
frontend/src/pages/Home.tsx (3)
27-27: New Modal component import looks good.This import is necessary for the new event modal functionality being added to the Home page.
37-37: State implementation for modal functionality is correct.The
modalOpenIndexstate variable with initial value ofnullproperly manages which event modal is currently open (if any).
136-144: Good implementation of event card with modal trigger.Replacing the anchor tag with a button that triggers the modal provides a better user experience by allowing users to view event details without leaving the page.
frontend/src/api/queries/homeQueries.ts (1)
67-77: GraphQL query enhanced with appropriate event fields.The addition of the new fields (
country,description,key,postalCode,region,summary) to the upcoming events query ensures the frontend can access the enriched event data from the backend.backend/apps/owasp/models/event.py (3)
7-11: New imports support AI processing capabilities.These imports provide the necessary functionality for geocoding, AI processing, and text manipulation that are required by the new methods in the Event model.
36-54: New geographic and descriptive fields enhance the Event model.The addition of these fields (
country,latitude,longitude,postal_code,region,summary,suggested_location) appropriately extends the Event model to support AI-enhanced location data and descriptive information.
161-171: Geo string function implementation is good.The
get_geo_stringmethod effectively combines location information with proper delimiter handling using thejoin_valuesutility.
frontend/src/types/modal.ts
Outdated
| onClose: () => void | ||
| button: ButtonType | ||
| children?: React.ReactNode | ||
| entityType: string |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Required property added without default value
You've added entityType as a required property to the ModalProps interface. This is a breaking change that will require updates to all existing Modal component usages. Consider making this property optional (entityType?: string) or providing a default value in the Modal component implementation.
🏁 Script executed:
#!/bin/bash
# Check for all usages of the Modal component to ensure they provide the entityType prop
rg "Modal\s*\(" --type tsx --type jsx -A 10 -B 5Length of output: 78
🏁 Script executed:
#!/bin/bash
# Searching for all usages of the Modal component across TSX and JSX files by explicitly defining these file types
rg --type-add='tsx:*.tsx' --type-add='jsx:*.jsx' "Modal\s*\(" -A 10 -B 5Length of output: 7438
Breaking Change: Modal 'entityType' Prop Lacks Default Value
- Location:
frontend/src/types/modal.ts, Line 12 - The
ModalPropsinterface now includes a requiredentityType: stringwith no default. - Our scan (e.g., within
frontend/__tests__/unit/pages/Modal.test.tsx) indicates that Modal usages (e.g., viarenderModal()) aren’t explicitly supplying this prop, which may lead to runtime errors or unexpected behavior.
Recommendation: Either make the entityType property optional (entityType?: string) or provide a default value within the Modal component implementation to maintain backward compatibility.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/apps/owasp/models/event.py (1)
163-172: Remove unused parameter from get_geo_string method.The
include_nameparameter is defined but never used in the function body.-def get_geo_string(self, include_name=True): +def get_geo_string(self): """Return geo string.""" return join_values( ( f"Name: {self.name}", f"Description: {self.description}", f"Summary: {self.summary}", ), delimiter=NL, )
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/apps/owasp/admin.py(1 hunks)backend/apps/owasp/graphql/nodes/event.py(1 hunks)backend/apps/owasp/management/commands/owasp_enrich_events.py(1 hunks)backend/apps/owasp/migrations/0025_remove_event_country_remove_event_postal_code_and_more.py(1 hunks)backend/apps/owasp/models/event.py(3 hunks)frontend/src/api/queries/homeQueries.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- backend/apps/owasp/migrations/0025_remove_event_country_remove_event_postal_code_and_more.py
🚧 Files skipped from review as they are similar to previous changes (3)
- backend/apps/owasp/graphql/nodes/event.py
- backend/apps/owasp/management/commands/owasp_enrich_events.py
- frontend/src/api/queries/homeQueries.ts
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: CodeQL (python)
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: Run frontend unit tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run backend tests
🔇 Additional comments (4)
backend/apps/owasp/models/event.py (3)
143-161: Add error handling to the generate_summary method.The implementation doesn't handle potential OpenAI API errors, which could cause exceptions.
Apply this diff to handle potential failures gracefully:
def generate_summary(self, prompt): """Generate a summary for the event using OpenAI.""" if not prompt: return "" open_ai = OpenAi() + if not self.description: + return "" open_ai.set_input( join_values( ( f"Name: {self.name}", f"Description: {self.description}", f"Dates: {self.start_date} - {self.end_date}", ), delimiter=NL, ) ) open_ai.set_max_tokens(100).set_prompt(prompt) - summary = open_ai.complete() - self.summary = summary if summary and summary != "None" else "" + try: + summary = open_ai.complete() + self.summary = summary if summary and summary != "None" else "" + except Exception as e: + # Log the error or handle it appropriately + self.summary = "" return self.summary
185-195: Add error handling to the generate_suggested_location method.Similar to the other AI methods, this implementation doesn't handle potential API errors from OpenAI.
Add try-except blocks to handle potential OpenAI API errors:
def generate_suggested_location(self, open_ai=None, max_tokens=100): """Generate project summary.""" if not (prompt := Prompt.get_owasp_event_suggested_location()): return open_ai = open_ai or OpenAi() open_ai.set_input(self.get_geo_string()) open_ai.set_max_tokens(max_tokens).set_prompt(prompt) - suggested_location = open_ai.complete() - self.suggested_location = ( - suggested_location if suggested_location and suggested_location != "None" else "" - ) + try: + suggested_location = open_ai.complete() + self.suggested_location = ( + suggested_location if suggested_location and suggested_location != "None" else "" + ) + except Exception as e: + # Log the error or handle it appropriately + self.suggested_location = ""
174-184: The generate_geo_location method looks good.This method properly attempts to first use the suggested location for coordinates, then falls back to the geo string if needed.
backend/apps/owasp/admin.py (1)
69-72: Good addition of suggested_location to the admin display.This change appropriately updates the admin interface to display the new geographical information from events.
arkid15r
left a comment
arkid15r
left a comment
There was a problem hiding this comment.
Good work, please look into addressing the bot's and the following comments when you get a chance:
frontend/src/components/Modal.tsx
Outdated
| </DialogHeader> | ||
| <Text className="mb-1 text-xs text-gray-700 dark:text-gray-300/60"> | ||
| The issue summary and the recommended steps to address it have been generated by AI | ||
| The {entityType} summary and the recommended steps to address it have been generated by AI |
There was a problem hiding this comment.
This is unique entirely for different types of modal (issue, event). Pass the full text instead of entity
frontend/src/pages/Home.tsx
Outdated
| <div className="mr-4 flex items-center"> | ||
| <FontAwesomeIcon icon={faCalendar} className="mr-2 h-4 w-4" /> | ||
| <span>{formatDateRange(event.startDate, event.endDate)}</span> | ||
| </div> |
There was a problem hiding this comment.
We want to show the suggested location here too.
| name: string | ||
| startDate: string | ||
| url: string | ||
| summary?: string |
There was a problem hiding this comment.
It seems we missed suggestedLocation
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/apps/github/graphql/nodes/release.py (1)
18-26: Consider updating fields tuple in Meta classWhile the
urlfield works correctly with the resolver, it's not included in thefieldstuple in the Meta class. Consider adding it there for consistency, even though GraphQL fields with resolvers don't strictly require it.class Meta: model = Release fields = ( "author", "is_pre_release", "name", "published_at", "tag_name", + "url", )
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
backend/poetry.lockis excluded by!**/*.lockfrontend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlbackend/poetry.lockis excluded by!**/*.lockfrontend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (17)
frontend/src/api/queries/homeQueries.ts(1 hunks)frontend/src/pages/Home.tsx(3 hunks)frontend/package.json(2 hunks)frontend/src/pages/Home.tsx(2 hunks)frontend/src/api/queries/homeQueries.ts(1 hunks).github/workflows/run-ci-cd.yaml(2 hunks)backend/apps/github/graphql/nodes/release.py(2 hunks)backend/apps/github/models/release.py(1 hunks)backend/tests/github/graphql/nodes/release_test.py(1 hunks)frontend/package.json(3 hunks)frontend/src/api/queries/homeQueries.ts(1 hunks)frontend/src/components/MultiSearch.tsx(7 hunks)frontend/src/components/UserCard.tsx(2 hunks)frontend/src/pages/Home.tsx(1 hunks)frontend/src/pages/Users.tsx(1 hunks)frontend/src/types/card.ts(1 hunks)frontend/src/types/search.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- frontend/src/api/queries/homeQueries.ts
- frontend/src/api/queries/homeQueries.ts
- frontend/src/pages/Home.tsx
- frontend/src/pages/Home.tsx
- frontend/src/pages/Home.tsx
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Run frontend e2e tests
- GitHub Check: CodeQL (python)
- GitHub Check: Run frontend unit tests
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: Run backend tests
🔇 Additional comments (21)
backend/apps/github/models/release.py (1)
53-56: Good addition of URL propertyThe
urlproperty implementation is clean and correctly constructs the release URL by combining the repository URL with the tag name. This provides a convenient way to access the full release URL without having to construct it manually each time.backend/tests/github/graphql/nodes/release_test.py (1)
28-28: Test updated appropriately for new fieldGood job updating the test expectations to include the new
urlfield. This ensures the GraphQL schema properly exposes the new field.backend/apps/github/graphql/nodes/release.py (2)
16-16: GraphQL schema field added correctlyThe
urlfield is properly added to the ReleaseNode class as a String type, which is appropriate for a URL.
32-34: Resolver implementation is correctThe implementation of
resolve_urlproperly returns the model'surlproperty value. The resolver naming and structure follow the pattern used by other resolvers in this class.frontend/src/types/card.ts (1)
51-52: Interface expanded to support additional user information.The
UserCardPropsinterface has been enhanced to include email and location fields, which will allow for richer user profile displays.frontend/src/pages/Users.tsx (1)
42-45: User card properties updated to include new fields.You've correctly implemented the changes to pass the email and location data to the UserCard component, which aligns with the interface updates.
frontend/src/types/search.ts (3)
2-2: Added EventType import to support event data in search.This import enables the inclusion of event types in the search functionality, which is essential for the AI processing of events.
11-11: Added optional eventData property to MultiSearchBarProps.The new property allows passing event data to the search component, enabling event-based searching capabilities.
16-16: Extended Suggestion.hits to include EventType[].This modification allows the search suggestion system to display event results alongside other entity types, creating a more comprehensive search experience.
.github/workflows/run-ci-cd.yaml (1)
227-227: CI/CD pipeline updated to include frontend e2e tests as a dependency.Adding the frontend end-to-end tests as a dependency for both staging and production image builds ensures that the new event functionality is properly tested before deployment.
Also applies to: 369-369
frontend/src/api/queries/homeQueries.ts (1)
71-72: Good addition of summary and suggestedLocation fieldsThese new fields enhance the event data retrieved for the homepage, aligning with the PR objective of adding AI processing to events. The
summaryfield will likely contain AI-generated content, whilesuggestedLocationprovides additional geographical context.frontend/package.json (1)
21-21: Package updates look goodThese are appropriate minor version updates that typically include bug fixes and small improvements. The changes are necessary for maintaining up-to-date dependencies.
Also applies to: 23-23, 59-59, 61-61, 67-67, 75-75
frontend/src/components/UserCard.tsx (2)
6-6: Enhanced UserCard props to include email and locationThe component now accepts additional user information properties, which improves flexibility and data display options.
27-32: Improved text handling for user informationThe styling changes from
line-clamp-1tomax-w-[250px] truncateprovide better text overflow control. The conditional rendering now displays either company, location, or email, enhancing the information hierarchy.frontend/src/components/MultiSearch.tsx (7)
7-7: Good preparation for event search functionalityAdding necessary imports and the
eventDataprop properly sets up the component to handle event data in the search functionality.Also applies to: 16-16, 26-26
49-54: Type definition properly updated for eventsThe type definition for
hitsnow correctly includesEventType[], ensuring type safety when handling event data in search results.
58-67: Well-implemented event filtering logicThe implementation correctly handles filtering events based on the search query and gracefully handles the case when
eventDatais undefined using optional chaining and a fallback empty array.
75-75: Correctly updated dependency arrayAdding
eventDatato the dependency array ensures the debounced search function correctly reacts to changes in event data.
85-86: Good implementation of event handling in search resultsThe code properly handles clicks on event suggestions by opening the event URL in a new tab, which is appropriate for external event links.
Also applies to: 93-95
116-118: Type casting properly handledThe type casting ensures type safety when handling different entity types in the suggestion click handler.
194-195: Appropriate icon selection for eventsUsing the calendar icon for events provides clear visual differentiation in the search results.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
frontend/src/pages/Home.tsx (1)
138-139: Improved key usage for React list renderingChanging from index-based keys to keys derived from
event.nameimproves stability during list updates. This is a good React practice that helps maintain component state correctly when items are added, removed, or reordered.However, ensure that
event.namevalues are unique across all events to avoid potential key conflicts.Consider using a more guaranteed unique identifier if available:
-<div key={`card-${event.name}`}> +<div key={`card-${event.id || event.name}`}>backend/apps/owasp/models/event.py (2)
161-170: Include region in the geo string
Currently,regionis not appended in the returned string, which may reduce geocoding accuracy.def get_geo_string(self, include_name=True): """Return geo string.""" return join_values( ( self.name.replace("OWASP", "").strip() if include_name else "", + self.region, self.country, self.postal_code, ), delimiter=", ", )
172-182: Consider adding error handling
Ifget_location_coordinatesfails due to invalid data or network issues, lat/long won't be updated, and no logs will indicate a problem.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
backend/Makefile(1 hunks)backend/apps/owasp/models/event.py(3 hunks)frontend/__tests__/unit/pages/Modal.test.tsx(1 hunks)frontend/src/components/Modal.tsx(2 hunks)frontend/src/pages/Contribute.tsx(1 hunks)frontend/src/pages/Home.tsx(3 hunks)frontend/src/types/event.ts(1 hunks)frontend/src/types/modal.ts(1 hunks)frontend/src/pages/Home.tsx(2 hunks)backend/apps/owasp/models/event.py(4 hunks)frontend/src/pages/Home.tsx(1 hunks)backend/Makefile(1 hunks)backend/apps/owasp/models/event.py(3 hunks)frontend/__tests__/unit/pages/Modal.test.tsx(1 hunks)frontend/src/components/Modal.tsx(2 hunks)frontend/src/pages/Contribute.tsx(1 hunks)frontend/src/pages/Home.tsx(2 hunks)frontend/src/types/event.ts(1 hunks)frontend/src/types/modal.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (14)
- frontend/src/types/event.ts
- frontend/src/pages/Contribute.tsx
- frontend/src/types/modal.ts
- frontend/src/pages/Contribute.tsx
- backend/Makefile
- backend/Makefile
- frontend/src/pages/Home.tsx
- frontend/src/pages/Home.tsx
- frontend/src/types/event.ts
- frontend/tests/unit/pages/Modal.test.tsx
- frontend/src/types/modal.ts
- frontend/src/components/Modal.tsx
- frontend/src/components/Modal.tsx
- frontend/tests/unit/pages/Modal.test.tsx
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: CodeQL (python)
- GitHub Check: Run frontend unit tests
- GitHub Check: Run backend tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (11)
frontend/src/pages/Home.tsx (3)
151-154: Added suggested location displayThis implementation satisfies the requested feature to show suggested locations for events. The UI pattern is consistent with other metadata displays in the component.
158-158: Updated Modal key for consistencyUsing
event.namein the Modal key is consistent with the earlier key change and helps maintain proper component identity.
164-164: Added AI attribution for event summariesReplacing
entityTypewith a specific description about AI-generated summaries provides transparency to users about the source of information. This aligns well with the PR objective of adding AI processing to events.backend/apps/owasp/models/event.py (8)
7-7: Imports look correct
These new imports appear valid and appropriately placed.Also applies to: 9-9, 10-10
36-36: Country field addition
Using an empty string default is acceptable here. No improvements needed.
38-38: Description field
Storing the event's description as text is appropriate.
40-41: Coordinate fields
Latitude and longitude fields look correct for optional geodata storage.
43-49: Postal code & region fields
These additions are straightforward. No issues to flag.
51-51: Summary field
A text field for event summaries adds valuable information. Looks good.
52-54: Suggested location field
This field effectively supports AI-driven location suggestions.
149-159: Add try-except to handle OpenAI errors
This method doesn't handle potential API failures or network-related issues, which can cause unhandled exceptions.
backend/apps/owasp/models/event.py
Outdated
| location = get_location_coordinates(self.suggested_location) | ||
| if location is None: | ||
| location = get_location_coordinates(self.get_geo_string()) | ||
| if location: | ||
| self.latitude = location.latitude | ||
| self.longitude = location.longitude | ||
|
|
||
| def generate_suggested_location(self, open_ai=None, max_tokens=100): | ||
| """Generate project summary.""" | ||
| if not (prompt := Prompt.get_owasp_event_suggested_location()): | ||
| return |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove duplicate logic and fix the docstring
Lines 183–193 replicate the same OpenAI call found again in lines 194–203, but lack error handling. Consolidate them into a single block with proper exception handling, and correct the docstring to accurately describe the method purpose.
def generate_suggested_location(self, open_ai=None, max_tokens=100):
- """Generate project summary."""
- if not (prompt := Prompt.get_owasp_event_suggested_location()):
- return
+ """Generate a suggested location for the event using OpenAI."""
- open_ai = open_ai or OpenAi()
- open_ai.set_input(self.get_geo_string())
- open_ai.set_max_tokens(max_tokens).set_prompt(prompt)
- suggested_location = open_ai.complete()
- self.suggested_location = (
- suggested_location if suggested_location and suggested_location != "None" else ""
- )
+ prompt = Prompt.get_owasp_event_suggested_location()
+ if not prompt:
+ return
open_ai = open_ai or OpenAi()
open_ai.set_input(self.get_geo_string())
open_ai.set_max_tokens(max_tokens).set_prompt(prompt)
try:
suggested_location = open_ai.complete()
self.suggested_location = (
suggested_location if suggested_location and suggested_location != "None" else ""
)
except (ValueError, TypeError):
self.suggested_location = ""📝 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.
| location = get_location_coordinates(self.suggested_location) | |
| if location is None: | |
| location = get_location_coordinates(self.get_geo_string()) | |
| if location: | |
| self.latitude = location.latitude | |
| self.longitude = location.longitude | |
| def generate_suggested_location(self, open_ai=None, max_tokens=100): | |
| """Generate project summary.""" | |
| if not (prompt := Prompt.get_owasp_event_suggested_location()): | |
| return | |
| location = get_location_coordinates(self.suggested_location) | |
| if location is None: | |
| location = get_location_coordinates(self.get_geo_string()) | |
| if location: | |
| self.latitude = location.latitude | |
| self.longitude = location.longitude | |
| def generate_suggested_location(self, open_ai=None, max_tokens=100): | |
| """Generate a suggested location for the event using OpenAI.""" | |
| prompt = Prompt.get_owasp_event_suggested_location() | |
| if not prompt: | |
| return | |
| open_ai = open_ai or OpenAi() | |
| open_ai.set_input(self.get_geo_string()) | |
| open_ai.set_max_tokens(max_tokens).set_prompt(prompt) | |
| try: | |
| suggested_location = open_ai.complete() | |
| self.suggested_location = ( | |
| suggested_location if suggested_location and suggested_location != "None" else "" | |
| ) | |
| except (ValueError, TypeError): | |
| self.suggested_location = "" |
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
backend/apps/owasp/models/event.py (2)
142-152: Add error handling for the generate_geo_location method.The method should include try-except blocks to handle potential errors from the
get_location_coordinatesfunction, similar to how you've implemented error handling in the other AI-related methods.def generate_geo_location(self): """Add latitude and longitude data.""" location = None if self.suggested_location and self.suggested_location != "None": - location = get_location_coordinates(self.suggested_location) + try: + location = get_location_coordinates(self.suggested_location) + except Exception: + location = None if location is None: - location = get_location_coordinates(self.get_context()) + try: + location = get_location_coordinates(self.get_context()) + except Exception: + location = None if location: self.latitude = location.latitude self.longitude = location.longitude
177-177: Update the docstring for the get_context method.The current docstring "Return geo string" doesn't accurately describe the method's purpose. The method now returns a comprehensive context string including name, description, summary and optionally dates.
def get_context(self, include_dates=False): - """Return geo string.""" + """Return a context string with event details including name, description, and summary."""frontend/src/pages/Home.tsx (1)
135-169: Great implementation of event modal functionality with location display.The addition of the modal to display event details and the inclusion of suggested location information improves the user experience significantly. This addresses the previous review comment about showing the suggested location.
However, there are a few improvements to consider:
- Use more reliable unique keys for React elements:
-<div key={`card-${event.name}`}> +<div key={`card-${event.id || index}`}> -<Modal key={`modal-${event.name}`} +<Modal key={`modal-${event.id || index}`}
- Add accessibility attributes to the modal button:
<button className="mb-2 w-full text-left text-lg font-semibold text-blue-500 hover:underline" onClick={() => setModalOpenIndex(index)} + aria-haspopup="dialog" + aria-expanded={modalOpenIndex === index} >
- Handle potential missing event URLs:
-button={{ label: 'View Event', url: event.url }} +button={event.url ? { label: 'View Event', url: event.url } : undefined}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/apps/owasp/management/commands/owasp_enrich_events.py(1 hunks)backend/apps/owasp/models/event.py(3 hunks)frontend/__tests__/e2e/pages/Home.spec.ts(1 hunks)frontend/src/components/ItemCardList.tsx(1 hunks)frontend/src/components/LogoCarousel.tsx(2 hunks)frontend/src/pages/Home.tsx(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/apps/owasp/management/commands/owasp_enrich_events.py
- frontend/tests/e2e/pages/Home.spec.ts
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Run backend tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: CodeQL (python)
- GitHub Check: Run frontend unit tests
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (3)
frontend/src/components/ItemCardList.tsx (1)
21-21: Removal of height constraint improves content flexibility.Removing the
h-64class allows the content container to adjust its height based on the content rather than being fixed at 16rem. This is a good change as it makes the component more adaptable while still maintaining the scrollable behavior withoverflow-y-auto.frontend/src/components/LogoCarousel.tsx (1)
55-79: Text improvements enhance clarity and readability.The text updates improve the clarity of the message and better organize the content by:
- Making the corporate supporters description more concise
- Creating a separate paragraph for the OWASP Nest project sponsorship information
- Simplifying the wording without losing the key information
These changes make the message clearer and more actionable for users.
frontend/src/pages/Home.tsx (1)
225-225: Heading size adjustment improves visual hierarchy.Changing the heading to a more appropriate size helps create a better visual hierarchy on the page.
* Added AI processing to events * Fix formatting error caused by merge conflict. * Fixed permissions * Update code * Added few changes * Update code --------- Co-authored-by: Arkadii Yakovets <[email protected]>
2 participants
Fixes: #1029