Remove admin guard in header #329
Conversation
Adjusted the header template to nest role checks within the signed-in condition. This ensures the 'Workspace' link only appears for signed-in admin users while maintaining accessibility for other user-specific links.
Added a log statement to inform when a previous task for a pull request is being canceled. This enhances transparency and helps with debugging by clearly indicating which tasks are terminated.
|
""" WalkthroughThe changes involve updates to both backend and frontend components. On the backend, error logging was enhanced to include full exception details, and scheduled tasks are now safely removed after iteration to prevent concurrent modification errors. An info-level logging statement was added before cancelling scheduled tasks for pull requests. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant HeaderComponent
User->>HeaderComponent: Loads header
HeaderComponent->>HeaderComponent: Check if user is signed in
alt User is signed in
HeaderComponent->>HeaderComponent: Render "Best practices" and "Teams" links
HeaderComponent->>HeaderComponent: Check if user is admin
alt User is admin
HeaderComponent->>HeaderComponent: Render "Workspace" link
end
end
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (11)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
CodeRabbit Configuration File (
|
github-actions
bot
added
client
application-server
size:S
There was a problem hiding this comment.
Pull Request Overview
This PR refines the header conditional rendering to display admin-specific links only when appropriate and enhances task cancellation traceability in the backend.
- Frontend: Separates the admin check from the signed-in condition to ensure that only admin users see the “Workspace” link while general users now have access to “Activity” and “Teams.”
- Backend: Adds an informative log entry before a task cancellation to improve debugging and traceability.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| webapp/src/app/core/header/header.component.html | Refactored conditional rendering for admin-specific and general user links |
| server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/badpracticedetector/BadPracticeDetectorScheduler.java | Added logging for task cancellation events |
Comments suppressed due to low confidence (1)
server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/badpracticedetector/BadPracticeDetectorScheduler.java:153
- The new log statement improves traceability; please ensure its format and log level align with the project's established logging conventions.
logger.info("Cancelling previous task for pull request: {}", pullRequest.getId());
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
webapp/src/app/core/header/header.component.html (1)
10-13: Conditional rendering logic looks correct
The new@if (signedIn())wrapper with a nested@if (user()?.roles?.includes('admin'))for “Workspace” cleanly separates admin-only content from links shown to all signed-in users. This aligns perfectly with the PR objectives.Consider adding unit or integration tests (e.g., via TestBed) to cover header link visibility in three scenarios: signed-out, signed-in non-admin, and signed-in admin.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/badpracticedetector/BadPracticeDetectorScheduler.java(1 hunks)webapp/src/app/core/header/header.component.html(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Verify API Specs and Client of the Application Server (add autocommit-openapi label to PR to auto...
- GitHub Check: Verify API Specs and Client of the Intelligence Service (add autocommit-openapi label to PR to au...
- GitHub Check: Webhook Ingest / Build linux/amd64 Docker Image for ghcr.io/ls1intum/hephaestus/webhook-ingest
- GitHub Check: Webhook Ingest / Build linux/arm64 Docker Image for ghcr.io/ls1intum/hephaestus/webhook-ingest
- GitHub Check: Intelligence Service / Build linux/arm64 Docker Image for ghcr.io/ls1intum/hephaestus/intelligence-service
- GitHub Check: Webapp / Build linux/arm64 Docker Image for ghcr.io/ls1intum/hephaestus/webapp
- GitHub Check: Webapp / Build linux/amd64 Docker Image for ghcr.io/ls1intum/hephaestus/webapp
- GitHub Check: Application Server / Build linux/arm64 Docker Image for ghcr.io/ls1intum/hephaestus/application-server
- GitHub Check: Application Server / Build linux/amd64 Docker Image for ghcr.io/ls1intum/hephaestus/application-server
- GitHub Check: Verify API Specs and Client of the Intelligence Service (add autocommit-openapi label to PR to au...
...ava/de/tum/in/www1/hephaestus/activity/badpracticedetector/BadPracticeDetectorScheduler.java
Show resolved
Hide resolved
Previously, the code did not check if `this.collapsibleTrigger` was defined before accessing its state, potentially leading to runtime errors. This fix ensures the trigger is validated for null or undefined before proceeding, improving stability.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
webapp/src/app/user/pull-request-bad-practice-card/pull-request-bad-practice-card.component.ts (1)
108-108: Good defensive programming!This null check is a valuable addition that prevents potential runtime errors when accessing
collapsibleTrigger.state(). It ensures the component behaves correctly when the ViewChild reference might not be available.Consider using optional chaining for a more concise implementation:
- if (this.collapsibleTrigger != undefined && this.collapsibleTrigger.state() === 'closed') { + if (this.collapsibleTrigger?.state() === 'closed') {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
webapp/src/app/user/pull-request-bad-practice-card/pull-request-bad-practice-card.component.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: Verify API Specs and Client of the Application Server (add autocommit-openapi label to PR to auto...
- GitHub Check: Verify API Specs and Client of the Intelligence Service (add autocommit-openapi label to PR to au...
- GitHub Check: Intelligence Service / Build linux/amd64 Docker Image for ghcr.io/ls1intum/hephaestus/intelligence-service
- GitHub Check: Webhook Ingest / Build linux/amd64 Docker Image for ghcr.io/ls1intum/hephaestus/webhook-ingest
- GitHub Check: Intelligence Service / Build linux/arm64 Docker Image for ghcr.io/ls1intum/hephaestus/intelligence-service
- GitHub Check: Webhook Ingest / Build linux/arm64 Docker Image for ghcr.io/ls1intum/hephaestus/webhook-ingest
- GitHub Check: Application Server / Build linux/amd64 Docker Image for ghcr.io/ls1intum/hephaestus/application-server
- GitHub Check: Application Server / Build linux/arm64 Docker Image for ghcr.io/ls1intum/hephaestus/application-server
- GitHub Check: Webapp / Build linux/arm64 Docker Image for ghcr.io/ls1intum/hephaestus/webapp
- GitHub Check: Webapp / Build linux/amd64 Docker Image for ghcr.io/ls1intum/hephaestus/webapp
- GitHub Check: Verify API Specs and Client of the Intelligence Service (add autocommit-openapi label to PR to au...
- GitHub Check: Run Chromatic
- GitHub Check: Verify API Specs and Client of the Application Server (add autocommit-openapi label to PR to auto...
Refactor task removal to avoid ConcurrentModificationException. Introduced a temporary list to collect completed or canceled tasks and remove them in bulk, ensuring safer iteration over scheduled tasks.
Replaced e.getMessage() with e.toString() in the error log to provide more comprehensive details about the exception. This ensures better debugging and traceability when issues occur.
Extended the DetectorRequest model with pull request number and repository name fields. Updated all related components, including data handling, API definitions, and UI routes, to support the new attributes. Adjusted email templates and dashboard links for consistency with the revised structure.
github-actions
bot
added
intelligence-service
size:L
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
server/intelligence-service/app/detector/bad_practice_detector.py (1)
81-83: Consider using structured trace attributes instead of string tagsWhile the implementation works, consider using structured trace attributes instead of converting the pull request number to a string in a tag list. This would provide more semantic meaning and better querying capabilities in the tracing system.
-langfuse_context.update_current_trace( - tags=[repository_name, str(pull_request_number)] -) +langfuse_context.update_current_trace( + tags=[repository_name], + metadata={"pull_request_number": pull_request_number} +)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/badpracticedetector/PullRequestBadPracticeDetector.java(1 hunks)server/application-server/src/main/java/de/tum/in/www1/hephaestus/intelligenceservice/model/DetectorRequest.java(5 hunks)server/application-server/src/main/resources/mail-templates/bad-practices-detected.html(1 hunks)server/intelligence-service/app/detector/bad_practice_detector.py(2 hunks)server/intelligence-service/app/routers/detector.py(2 hunks)server/intelligence-service/openapi.yaml(1 hunks)webapp/src/app/app.routes.ts(1 hunks)webapp/src/app/core/header/header.component.html(1 hunks)webapp/src/app/home/activity/activity-dashboard.component.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- server/application-server/src/main/resources/mail-templates/bad-practices-detected.html
🚧 Files skipped from review as they are similar to previous changes (1)
- webapp/src/app/core/header/header.component.html
🧰 Additional context used
🪛 GitHub Actions: Intelligence Service QA
server/intelligence-service/app/detector/bad_practice_detector.py
[error] 1-1: Black formatting check failed. File would be reformatted. Run 'black' to fix code style issues.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Webapp / Create and Push Multi-Arch Manifest
- GitHub Check: Application Server / Build linux/amd64 Docker Image for ghcr.io/ls1intum/hephaestus/application-server
- GitHub Check: Application Server / Build linux/arm64 Docker Image for ghcr.io/ls1intum/hephaestus/application-server
- GitHub Check: Mend Security Check
🔇 Additional comments (12)
webapp/src/app/app.routes.ts (1)
50-51: LGTM! Routes properly configured for wider accessibility.The new routes for best practices are correctly configured to be accessible by all authenticated users (via AuthGuard) rather than just admins. Both the user-specific and general routes use the same component which is consistent with the PR objective of improving conditional rendering.
webapp/src/app/home/activity/activity-dashboard.component.ts (1)
30-30: LGTM! Improved parameter handling for flexible routing.The changes enhance the component's flexibility:
- The type change from
string | nulltostring | undefinedis more appropriate- The fallback logic properly handles both user-specific and general routes
This implementation supports the route changes in app.routes.ts, allowing the component to work whether accessed via a user-specific path or the general best practices path.
Also applies to: 38-38
server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/badpracticedetector/PullRequestBadPracticeDetector.java (1)
66-67: Changes properly propagate repository context informationThese new fields will provide valuable context to the detector service, allowing for better tracing and debugging capabilities with repository-specific information.
server/intelligence-service/openapi.yaml (1)
50-55: API specification properly updated with new fieldsThe OpenAPI schema is correctly updated with the new required fields, maintaining consistency with the backend changes.
server/intelligence-service/app/routers/detector.py (2)
15-16: Data model correctly updated with new fieldsThe DetectorRequest model has been properly extended with the new fields that match the OpenAPI specification.
37-38: New parameters are properly passed to the detector functionThe changes correctly propagate the new fields from the request to the detector function.
server/application-server/src/main/java/de/tum/in/www1/hephaestus/intelligenceservice/model/DetectorRequest.java (6)
39-41: Good addition of new fields to JsonPropertyOrder annotationThe new properties
PULL_REQUEST_NUMBERandREPOSITORY_NAMEare properly added to the JsonPropertyOrder annotation, ensuring they'll be serialized in the correct order.
57-62: New property constants and fields properly definedThe JSON property constants and corresponding fields for
pullRequestNumberandrepositoryNamefollow the established pattern in the class. The type choices (Integer for PR number and String for repository name) are appropriate for their purposes.
177-226: Well-implemented accessors for new fieldsThe fluent setters, getters, and standard setters for both new fields follow the same pattern as existing methods in the class. All necessary annotations (@JsonProperty, @JsonInclude, @jakarta.annotation.Nonnull) are correctly applied, maintaining consistency with the rest of the codebase.
265-267: Properly updated equals methodThe equals method has been correctly updated to include comparisons for the new fields, maintaining proper object equality semantics.
272-272: Correctly updated hashCode methodThe hashCode method properly includes the new fields, ensuring consistent hashing behavior that aligns with the updated equals method.
283-285: toString method properly updatedThe toString implementation now includes the new fields, which helps with debugging and logging by providing a complete string representation of the object.
server/intelligence-service/app/detector/bad_practice_detector.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull Request Overview
This pull request removes the admin-only restriction on header links and updates both backend and frontend components to support improved bad practice detection through extended traceability fields and refined scheduling logic. Key changes include updating error logging and scheduling in the backend, modifying frontend routes and header links, and extending the DetectorRequest model with new repository tracking fields.
Reviewed Changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| webapp/src/app/user/pull-request-bad-practice-card/pull-request-bad-practice-card.component.ts | Added a null check for collapsibleTrigger to prevent runtime errors |
| webapp/src/app/home/activity/activity-dashboard.component.ts | Updated userLogin assignment to default to the current user if not provided |
| webapp/src/app/core/header/header.component.html | Revised header to remove the admin guard and include a “Best practices” link |
| webapp/src/app/app.routes.ts | Updated routes to use best practices paths instead of activity paths |
| server/intelligence-service/app/routers/detector.py | Added repository_name and pull_request_number fields to the DetectorRequest model |
| server/intelligence-service/app/detector/bad_practice_detector.py | Updated the detection function to include repository tracking in trace tags |
| server/application-server/src/main/resources/mail-templates/bad-practices-detected.html | Simplified the email template’s review link to a generic best practices dashboard link |
| server/application-server/src/main/java/de/tum/in/www1/hephaestus/intelligenceservice/model/DetectorRequest.java | Extended the DetectorRequest model with new non-nullable repository fields |
| server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/badpracticedetector/PullRequestBadPracticeDetector.java | Populated new repository fields from the pull request object |
| server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/badpracticedetector/BadPracticeDetectorScheduler.java | Enhanced error logging and task cancellation logic to improve scheduling |
Files not reviewed (1)
- server/intelligence-service/openapi.yaml: Language not supported
This pull request introduces several updates to enhance functionality, improve error handling, and streamline the user interface for bad practice detection in pull requests. The changes span across the backend, frontend, and email templates, focusing on adding new fields for better traceability, refining scheduling logic, and updating user-facing routes and templates.
Backend Enhancements:
Improved Error Logging:
checkUserRoleAndScheduleDetectionAtTimeto usee.toString()for more detailed exception information. (BadPracticeDetectorScheduler.java, server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/badpracticedetector/BadPracticeDetectorScheduler.javaL130-R130)Enhanced Scheduling Logic:
scheduleDetectionAtTimeto prevent concurrent task cancellations by introducing a temporary list for tasks to remove. (BadPracticeDetectorScheduler.java, server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/badpracticedetector/BadPracticeDetectorScheduler.javaR151-R160)Extended
DetectorRequestModel:repositoryNameandpullRequestNumberfields toDetectorRequestfor better traceability in bad practice detection. (PullRequestBadPracticeDetector.java, [1];DetectorRequest.java, [2] [3] [4] [5] [6]openapi.yaml, server/intelligence-service/openapi.yamlR50-R64)Python Service Updates:
detect_bad_practicesfunction to accept and utilizerepository_nameandpull_request_numberfor tagging and tracking. (bad_practice_detector.py, [1] [2]DetectorRequestin Python to include these fields. (detector.py, [1] [2]Frontend and UI Updates:
Route Adjustments:
app.routes.ts, webapp/src/app/app.routes.tsL50-R51)Header and Dashboard Updates:
header.component.html, webapp/src/app/core/header/header.component.htmlL10-R14)ActivityDashboardComponentto ensureuserLogindefaults to the current user if noidis provided in the route. (activity-dashboard.component.ts, webapp/src/app/home/activity/activity-dashboard.component.tsL30-R38)Bug Fix:
collapsibleTriggerinPullRequestBadPracticeCardComponentto prevent runtime errors. (pull-request-bad-practice-card.component.ts, webapp/src/app/user/pull-request-bad-practice-card/pull-request-bad-practice-card.component.tsL108-R108)Email Template Update:
bad-practices-detected.html, server/application-server/src/main/resources/mail-templates/bad-practices-detected.htmlL12-R15)