-
Notifications
You must be signed in to change notification settings - Fork 9
Do not switch to ported APIs yet #464
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
Do not switch to ported APIs yet #464
Conversation
Signed-off-by: Lukasz Gryglicki <[email protected]>
WalkthroughThe service methods in cla-contributor.service.ts were updated to call v2 API endpoints instead of v3/v4, with prior endpoints left as commented lines and TODO notes. No public API signatures changed. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
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
🧹 Nitpick comments (2)
src/app/core/services/cla-contributor.service.ts (2)
153-156: DRY: delegate to getProject(projectId) for Gerrit project infoThis endpoint and return type match getProject; delegating avoids duplication and keeps behaviors in sync.
Apply this diff:
- // LG:TODO - const url = this.getV2Endpoint('/v2/project/' + projectId); - // const url = this.getV4Endpoint('/v4/project-compat/' + projectId); - return this.httpClient.get<ProjectModel>(url); + return this.getProject(projectId);
59-61: Replace “LG:TODO” with actionable TODOs referencing an issue/ticket“LG:TODO” is ambiguous and easy to miss. Use a standardized format (e.g., TODO(username): short action… [JIRA-123]) so the intent and follow-up are clear and traceable.
Also applies to: 75-79, 83-86, 153-156
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/app/core/services/cla-contributor.service.ts(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/app/core/services/cla-contributor.service.ts (2)
src/app/core/models/project.ts (1)
ProjectModel(4-22)src/app/core/models/active-signature.ts (1)
ActiveSignatureModel(4-10)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: snyk-scan-npm-pr
- GitHub Check: yarn-scan-cypress-pr
- GitHub Check: snyk-scan-edge-npm-pr
🔇 Additional comments (2)
src/app/core/services/cla-contributor.service.ts (2)
83-86: EncodeuserIdin the URL path and validate the response modelPlease update the endpoint construction to safely encode the
userId, and manually confirm that the JSON returned by/v2/user/{id}/active-signaturematches ourActiveSignatureModel(field names, types, and casing).– src/app/core/services/cla-contributor.service.ts (around line 83)
- const url = this.getV2Endpoint('/v2/user/' + userId + '/active-signature'); + const url = this.getV2Endpoint( + `/v2/user/${encodeURIComponent(userId)}/active-signature` + ); return this.httpClient.get<ActiveSignatureModel>(url);
- Verify that the API’s response payload shape aligns with
ActiveSignatureModel.
75-79: Verify v2/projectresponse shape & encodeprojectIdPlease ensure the v2 endpoint still returns a CLA-Group payload matching
ProjectModel(i.e. includesprojects,foundation_sfid,project_sfid, etc.) or add an adapter if it differs. Also URL-encode theprojectIdto avoid path issues.• File:
src/app/core/services/cla-contributor.service.ts
Lines 75–79Proposed change:
- const url = this.getV2Endpoint('/v2/project/' + projectId); + const url = this.getV2Endpoint(`/v2/project/${encodeURIComponent(projectId)}`);
cc @ahmedomosanya @mlehotskylf @ah-med
Summary by CodeRabbit