Skip to content
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

ADM-727:[backend][docs]feat: Merge two API into one #897

Merged
merged 7 commits into from
Jan 10, 2024
Merged

ADM-727:[backend][docs]feat: Merge two API into one #897

merged 7 commits into from
Jan 10, 2024

Conversation

Lei010
Copy link

@Lei010 Lei010 commented Jan 9, 2024

Summary

...

Before

Generate Report divided into two apis.

Screenshots
If applicable, add screenshots to help explain behavior of your code.

After

Merge two API into one, and asyncly generate pipeline and code base metrics.

Screenshots
If applicable, add screenshots to help explain behavior of your code.

Note

Null

Copy link

codacy-production bot commented Jan 9, 2024

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation Diff coverage
+0.00% 100.00%
Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (fb3d989) 4915 4915 100.00%
Head commit (3a66895) 9908 (+4993) 9908 (+4993) 100.00% (+0.00%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details
Coverable lines Covered lines Diff coverage
Pull request (#897) 3 3 100.00%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

You may notice some variations in coverage metrics with the latest Coverage engine update. For more details, visit the documentation

Copy link

github-actions bot commented Jan 9, 2024

Hi @Lei010! 👋
Thank you for submitting a pull request! We appreciate your contribution and will review your changes as soon as possible.

Copy link
Collaborator

@guzhongren guzhongren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

must be Changed with the comments

@@ -62,7 +65,7 @@ public ResponseEntity<ReportResponse> generateReport(@PathVariable String report
return ResponseEntity.status(HttpStatus.OK).body(reportResponse);
}

@PostMapping("/board")
@PostMapping("/v1/board")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

try {
saveReporterInHandler(generateReporter(request), IdUtil.getBoardReportId(request.getCsvTimeStamp()));
updateMetricsDataReadyInHandler(request.getCsvTimeStamp(), request.getMetrics());
log.info(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where is the log of start to do something...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

IdUtil.getDoraReportId(pipelineRequest.getCsvTimeStamp()));
updateMetricsDataReadyInHandler(pipelineRequest.getCsvTimeStamp(), pipelineRequest.getMetrics());
log.info(
"Successfully generate pipeline report, _metrics: {}, _considerHoliday: {}, _startTime: {}, _endTime: {}, _pipelineReportId: {}",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where is the start to do something,....

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

IdUtil.getCodeBaseReportId(codebaseRequest.getCsvTimeStamp()));
updateMetricsDataReadyInHandler(codebaseRequest.getCsvTimeStamp(), codebaseRequest.getMetrics());
log.info(
"Successfully generate codebase report, _metrics: {}, _considerHoliday: {}, _startTime: {}, _endTime: {}, _codeBaseReportId: {}",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where is the start to do something,....

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -161,7 +161,7 @@ void shouldReturnAcceptedStatusAndCallbackUrlAndIntervalWhenCallBoardReports() t
doNothing().when(generateReporterService).updateMetricsDataReadyInHandler(any(), any());

MockHttpServletResponse response = mockMvc
.perform(post("/reports/board").contentType(MediaType.APPLICATION_JSON)
.perform(post("/reports/v1/board").contentType(MediaType.APPLICATION_JSON)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove v1

is there any design for the new endpoint? i think we should follow the design

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

GenerateReportRequest.class);
ReportResponse reportResponse = mapper.readValue(new File(RESPONSE_FILE_PATH), ReportResponse.class);
reportRequest.setCsvTimeStamp("1683734399999");
String boardTimeStamp = "board-1683734399999";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is 1683734399999?
can we refactor with a readable name like board-20240109232359, even add milliseconds

must be changed with this comment

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

ReportResponse reportResponse = mapper.readValue(new File(RESPONSE_FILE_PATH), ReportResponse.class);
GenerateReporterService spyGenerateReporterService = spy(generateReporterService);
reportRequest.setCsvTimeStamp("1683734399999");
String doraTimeStamp = "dora-1683734399999";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as board

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

ReportResponse reportResponse = mapper.readValue(new File(RESPONSE_FILE_PATH), ReportResponse.class);
GenerateReporterService spyGenerateReporterService = spy(generateReporterService);
reportRequest.setCsvTimeStamp("1683734399999");
String codebaseTimeStamp = "github-1683734399999";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as board

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@Lei010 Lei010 changed the title ADM-741:[backend]feat: remove plain text token ADM-727:[backend]feat: remove plain text token Jan 10, 2024
@Lei010 Lei010 changed the title ADM-727:[backend]feat: remove plain text token ADM-727:[backend][docs]feat: remove plain text token Jan 10, 2024
@Lei010 Lei010 changed the title ADM-727:[backend][docs]feat: remove plain text token ADM-727:[backend][docs]feat: Merge two API into one Jan 10, 2024
Copy link

@mikeyangyun mikeyangyun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Lei010 Lei010 merged commit 75383d2 into main Jan 10, 2024
42 checks passed
@Lei010 Lei010 deleted the ADM-727 branch January 10, 2024 08:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants