-
Notifications
You must be signed in to change notification settings - Fork 307
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
Development
: Cleanup URL prefix migration
#10435
Conversation
WalkthroughThis pull request updates API endpoint references across both backend and frontend code. The changes convert static paths and package names into dynamic, template-based formats using Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant WebConfig
participant ToolsInterceptor
participant Controller
Client->>WebConfig: Send request to /api/{module}/public/...
WebConfig->>ToolsInterceptor: Check exclusion pattern (/api/*/public/**)
ToolsInterceptor-->>WebConfig: Confirm exclusion
WebConfig->>Controller: Forward request for processing
Controller-->>Client: Return response
sequenceDiagram
participant Component
participant Service
participant APIServer
Component->>Service: Invoke create/update/delete with courseId and data
Service->>Service: Build endpoint via getResourceEndpoint(courseId, data)
Service->>APIServer: Send HTTP request to constructed URL
APIServer-->>Service: Send response
Service-->>Component: Return processed response
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 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 (8)
🪧 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: 0
🧹 Nitpick comments (1)
src/main/webapp/app/shared/metis/post.service.ts (1)
109-109
: Consider using getResourceEndpoint for consistency.While other methods use the
getResourceEndpoint
method, this method uses a hardcoded URL string. Consider refactoring to use the same pattern for better maintainability.- .put(`api/communication/courses/${courseId}/messages/${postId}/display-priority`, {}, { params: { displayPriority }, observe: 'response' }) + .put(`${this.getResourceEndpoint(courseId, undefined, undefined)}/${postId}/display-priority`, {}, { params: { displayPriority }, observe: 'response' })
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/main/java/de/tum/cit/aet/artemis/atlas/web/admin/AdminStandardizedCompetencyResource.java
(8 hunks)src/main/java/de/tum/cit/aet/artemis/core/config/Constants.java
(1 hunks)src/main/java/de/tum/cit/aet/artemis/core/config/WebConfigurer.java
(1 hunks)src/main/java/de/tum/cit/aet/artemis/core/security/annotations/EnforceAdmin.java
(1 hunks)src/main/java/de/tum/cit/aet/artemis/core/web/admin/AdminBuildJobQueueResource.java
(5 hunks)src/main/webapp/app/lti/lti13-exercise-launch.component.ts
(1 hunks)src/main/webapp/app/shared/metis/answer-post.service.ts
(4 hunks)src/main/webapp/app/shared/metis/post.service.ts
(6 hunks)src/test/java/de/tum/cit/aet/artemis/athena/service/connectors/AthenaFeedbackSuggestionsServiceTest.java
(1 hunks)src/test/javascript/spec/component/lti/lti13-exercise-launch.component.spec.ts
(6 hunks)src/test/javascript/spec/interceptor/errorhandler.interceptor.spec.ts
(1 hunks)src/test/javascript/spec/service/metis/post.service.spec.ts
(2 hunks)
✅ Files skipped from review due to trivial changes (3)
- src/main/java/de/tum/cit/aet/artemis/core/security/annotations/EnforceAdmin.java
- src/main/java/de/tum/cit/aet/artemis/atlas/web/admin/AdminStandardizedCompetencyResource.java
- src/main/java/de/tum/cit/aet/artemis/core/web/admin/AdminBuildJobQueueResource.java
🧰 Additional context used
📓 Path-based instructions (4)
`src/test/javascript/spec/**/*.ts`: jest: true; mock: NgMock...
src/test/javascript/spec/**/*.ts
: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}
src/test/javascript/spec/interceptor/errorhandler.interceptor.spec.ts
src/test/javascript/spec/component/lti/lti13-exercise-launch.component.spec.ts
src/test/javascript/spec/service/metis/post.service.spec.ts
`src/main/java/**/*.java`: naming:CamelCase; principles:{sin...
src/main/java/**/*.java
: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/core/config/Constants.java
src/main/java/de/tum/cit/aet/artemis/core/config/WebConfigurer.java
`src/main/webapp/**/*.ts`: angular_style:https://angular.io/...
src/main/webapp/app/lti/lti13-exercise-launch.component.ts
src/main/webapp/app/shared/metis/answer-post.service.ts
src/main/webapp/app/shared/metis/post.service.ts
`src/test/java/**/*.java`: test_naming: descriptive; test_si...
src/test/java/**/*.java
: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true
src/test/java/de/tum/cit/aet/artemis/athena/service/connectors/AthenaFeedbackSuggestionsServiceTest.java
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Call Build Workflow / Build .war artifact
- GitHub Check: Call Build Workflow / Build and Push Docker Image
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: client-tests-selected
- GitHub Check: client-tests
- GitHub Check: server-style
- GitHub Check: server-tests
- GitHub Check: Analyse
🔇 Additional comments (25)
src/main/webapp/app/lti/lti13-exercise-launch.component.ts (1)
58-58
:✅ Verification successful
Updated endpoint URL to follow the new API structure.
The endpoint URL has been updated from 'api/public/lti13/auth-login' to 'api/lti/public/lti13/auth-login', correctly following the new API structure where module names come before 'public' in the path.
🏁 Script executed:
#!/bin/bash # Verify that no references to the old API pattern remain in other LTI components echo "Checking for any remaining old API patterns in LTI components..." rg -n --glob "*.ts" "api/public/lti" src/main/webapp/app/Length of output: 196
I'll run an additional verification search to ensure that no references to the old endpoint pattern remain in the repository. This new script will search for any occurrence of the string
"api/public/lti13/auth-login"
to confirm that the update is applied consistently.
🏁 Script executed:
#!/bin/bash echo "Searching for any reference to the old API endpoint: 'api/public/lti13/auth-login' within the repository..." rg -n "api/public/lti13/auth-login" .Length of output: 268
Endpoint URL update verified and no remaining old references found.
The updated endpoint in
src/main/webapp/app/lti/lti13-exercise-launch.component.ts
is correctly structured asapi/lti/public/lti13/auth-login
, and our searches confirmed that no code still references the old API endpoint pattern.src/test/java/de/tum/cit/aet/artemis/athena/service/connectors/AthenaFeedbackSuggestionsServiceTest.java (1)
85-85
: Updated test to match new API endpoint structure.The test has been correctly updated to use the new API structure where module names come before 'public' in the path: '/api/athena/public/programming-exercises/' instead of '/api/public/athena/programming-exercises/'.
src/main/java/de/tum/cit/aet/artemis/core/config/WebConfigurer.java (1)
138-138
:❓ Verification inconclusive
Updated interceptor exclusion pattern to match the new API structure.
The excludePathPatterns has been updated to '/api/*/public/**' to match the new API structure where module names come before 'public' in the path. This is crucial for ensuring that all public endpoints are properly excluded from the tools interceptor.
🏁 Script executed:
#!/bin/bash # Verify that controllers are using the updated API pattern echo "Checking for any controller classes still using the old public API pattern..." rg -n --glob "*.java" "@RequestMapping\\(\"*/api/public/" src/main/java/Length of output: 237
Focus on verifying controller mapping consistency
The change at line 138 in
src/main/java/de/tum/cit/aet/artemis/core/config/WebConfigurer.java
correctly updates the interceptor exclusion pattern to'/api/*/public/**'
in line with the new API structure where module names precedepublic
. An automated search for occurrences of the old"/api/public/"
pattern produced no results; however, the output was inconclusive.
- Please verify manually that all controller endpoints now incorporate the module segment (e.g.,
/api/<module>/public/...
) and that no endpoints are still using the legacy"/api/public/"
path.src/main/java/de/tum/cit/aet/artemis/core/config/Constants.java (1)
73-73
:❓ Verification inconclusive
Updated constant to use the new API endpoint structure.
The ATHENA_PROGRAMMING_EXERCISE_REPOSITORY_API_PATH constant has been updated to follow the new URL structure '/api/athena/public/programming-exercises/' where module names come before 'public' in the path.
🏁 Script executed:
#!/bin/bash # Check for any remaining code using the old API pattern for Athena echo "Checking for any references to the old Athena API pattern..." rg -n --glob "*.java" "/api/public/athena" src/main/java/Length of output: 188
Manual Verification Required: Athena API Endpoint Update
The constant in
src/main/java/de/tum/cit/aet/artemis/core/config/Constants.java
(line 73) has been updated to use the new endpoint/api/athena/public/programming-exercises/
—reflecting the intended structure where module names appear before'public'
. Although a search for references to the old API pattern (/api/public/athena
) produced no output, the result is based on limited inference from the shell script output. Please manually verify that:
- The updated endpoint is used consistently across the codebase.
- No residual references to the old endpoint pattern exist.
src/test/javascript/spec/interceptor/errorhandler.interceptor.spec.ts (1)
49-50
: URL endpoint structure updated to follow new module-based patternThe test case has been updated to use the new URL structure with the
core
module prefix. This correctly reflects the API restructuring from flat paths (/api/public/...
) to a modular pattern (/api/{module}/public/...
).src/test/javascript/spec/component/lti/lti13-exercise-launch.component.spec.ts (6)
94-94
: URL endpoint updated to match new module-based patternThe API endpoint has been updated from
api/public/lti13/auth-login
toapi/lti/public/lti13/auth-login
, following the new convention where module name comes beforepublic
.
109-109
: URL endpoint updated to match new module-based patternSimilar to the previous change, the API endpoint follows the new module-based structure.
118-118
: URL endpoint updated to match new module-based patternConsistently updated to the new endpoint structure.
133-133
: URL endpoint updated to match new module-based patternConsistently updated to the new endpoint structure.
155-155
: URL endpoint updated to match new module-based patternConsistently updated to the new endpoint structure.
177-177
: URL endpoint updated to match new module-based patternConsistently updated to the new endpoint structure.
src/test/javascript/spec/service/metis/post.service.spec.ts (2)
94-94
: Hardcoded API path replaces dynamic constructionThe test now uses a specific API path with the module name instead of relying on the
service.resourceUrl
property. This aligns with the module-based URL restructuring pattern.
109-109
: Updated API path with module-specific endpointThe test now uses a specific communication module endpoint instead of a generic resource URL. This is consistent with the module-based restructuring approach.
src/main/webapp/app/shared/metis/answer-post.service.ts (4)
26-26
: URL construction now uses dynamic endpoint methodThe service now uses the
getResourceEndpoint
method for building URLs instead of a static resource URL, allowing for more dynamic module-based paths.
38-38
: URL construction updated to use dynamic endpoint methodConsistent with the previous update, using the new URL construction pattern.
49-49
: URL construction updated to use dynamic endpoint methodMaintains consistency with other methods in using the new URL construction pattern.
65-71
: New method for determining the appropriate API endpointThis new method intelligently selects the appropriate endpoint based on the post type:
- Uses communication module for conversation-related posts
- Uses plagiarism module for other posts
This approach aligns with the module-based URL restructuring and improves code organization.
src/main/webapp/app/shared/metis/post.service.ts (8)
31-31
: Refactoring to use getResourceEndpoint simplifies URL construction.The refactoring to use the
getResourceEndpoint
method centralizes URL creation logic, improving maintainability.
80-84
: LGTM! Consistent endpoint generation approach.The change properly utilizes the centralized
getResourceEndpoint
method for constructing the URL, maintaining consistency with other methods.
96-96
: URL construction is properly refactored.The update method now correctly uses the centralized endpoint generation approach.
120-120
: URL construction is properly refactored.The delete method now correctly uses the centralized endpoint generation approach.
152-154
: Improved method documentation.The JSDoc has been updated to better explain the purpose of the method, which improves code readability and maintainability.
155-155
: Documentation parameter update.The parameter documentation has been updated to reflect the new required parameter.
160-160
: Method signature improvement.Making
courseId
a required parameter improves type safety and makes the method's requirements clearer.
162-165
: Consistent dynamic URL construction.The return values now include properly constructed API paths that align with the URL prefix migration objective mentioned in the PR.
the change of server coverage thresholds for "exam" are also required on develop, it's unrelated to these changes |
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.
Tested for LTI on ts3
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.
Code lgtm 👍. Great work
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.
Approve code
Checklist
General
Motivation and Context
Summary by CodeRabbit
New Features
Documentation
Summary by CodeRabbit
Refactor
Documentation