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

Fix home page workflow instance miss status #15193

Merged
merged 1 commit into from
Nov 22, 2023

Conversation

ruanwenjun
Copy link
Member

@ruanwenjun ruanwenjun commented Nov 21, 2023

Purpose of the pull request

  • Fix home page miss some status
  • Remove some unused code.

Brief change log

Verify this pull request

This pull request is code cleanup without any test coverage.

(or)

This pull request is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(or)

If your pull request contain incompatible change, you should also add it to docs/docs/en/guide/upgrede/incompatible.md

@github-actions github-actions bot added UI ui and front end related backend document labels Nov 21, 2023
@ruanwenjun ruanwenjun force-pushed the dev_wenjun_fixHomePage branch 3 times, most recently from 59ec820 to 3131fe0 Compare November 21, 2023 06:26
@codecov-commenter
Copy link

codecov-commenter commented Nov 21, 2023

Codecov Report

Attention: 64 lines in your changes are missing coverage. Please review.

Comparison is base (7308888) 37.95% compared to head (683ed23) 37.83%.

❗ Current head 683ed23 differs from pull request most recent head 1c227e8. Consider uploading reports for the commit 1c227e8 to get more accurate results

Files Patch % Lines
...uler/api/service/impl/DataAnalysisServiceImpl.java 38.00% 28 Missing and 3 partials ⚠️
...scheduler/api/service/impl/ProjectServiceImpl.java 25.00% 6 Missing and 3 partials ⚠️
...heduler/api/controller/DataAnalysisController.java 45.45% 3 Missing and 3 partials ⚠️
...hinscheduler/api/vo/WorkflowDefinitionCountVo.java 25.00% 5 Missing and 1 partial ⚠️
...phinscheduler/alert/plugin/AlertPluginManager.java 0.00% 2 Missing ⚠️
...uler/api/controller/v2/StatisticsV2Controller.java 33.33% 2 Missing ⚠️
...e/dolphinscheduler/api/vo/TaskInstanceCountVo.java 84.61% 1 Missing and 1 partial ⚠️
...lphinscheduler/api/vo/WorkflowInstanceCountVo.java 84.61% 1 Missing and 1 partial ⚠️
...inscheduler/plugin/task/api/TaskPluginManager.java 0.00% 2 Missing ⚠️
...apache/dolphinscheduler/api/dto/DefineUserDto.java 50.00% 0 Missing and 1 partial ⚠️
... and 1 more
Additional details and impacted files
@@             Coverage Diff              @@
##                dev   #15193      +/-   ##
============================================
- Coverage     37.95%   37.83%   -0.12%     
+ Complexity     4671     4646      -25     
============================================
  Files          1278     1281       +3     
  Lines         44874    44861      -13     
  Branches       4870     4864       -6     
============================================
- Hits          17030    16974      -56     
- Misses        25988    26031      +43     
  Partials       1856     1856              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ruanwenjun ruanwenjun force-pushed the dev_wenjun_fixHomePage branch from 3131fe0 to 870d365 Compare November 21, 2023 09:09
@ruanwenjun ruanwenjun added the bug Something isn't working label Nov 21, 2023
songjianet
songjianet previously approved these changes Nov 22, 2023
return Result.success(defineUserDto);
public Result<WorkflowDefinitionCountVo> countDefinitionByUser(@Parameter(hidden = true) @RequestAttribute(value = Constants.SESSION_USER) User loginUser,
@RequestParam(value = "projectCode", required = false) Long projectCode) {
if (projectCode == null || projectCode == 0) {
Copy link
Member

Choose a reason for hiding this comment

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

Why this line needs check projectCode == 0, looks like logic is simily above

Copy link
Member Author

Choose a reason for hiding this comment

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

Since if from home page the projectCode will be 0... this should be refactor.

Copy link
Member Author

Choose a reason for hiding this comment

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

Have removed this logic.

<select id="countWorkflowInstanceStateByProjectCodes" resultType="org.apache.dolphinscheduler.dao.model.WorkflowInstanceStatusCountDto">
select state, count(0) as count
from t_ds_process_instance
where is_sub_process = 0
<if test="startTime != null and endTime != null">
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<if test="startTime != null and endTime != null">
<if test="startTime != null>

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

Copy link
Member

@qingwli qingwli left a comment

Choose a reason for hiding this comment

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

LGTM

@ruanwenjun
Copy link
Member Author

ruanwenjun commented Nov 22, 2023

LGTM

Thanks for your review

Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 5 Code Smells

55.9% 55.9% Coverage
0.0% 0.0% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

@ruanwenjun ruanwenjun merged commit df656a7 into apache:dev Nov 22, 2023
@ruanwenjun ruanwenjun deleted the dev_wenjun_fixHomePage branch November 22, 2023 02:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend bug Something isn't working document ready-to-merge UI ui and front end related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants