Skip to content

[Feature-8138][Task] add at@ msg in the dingtalk task plugin#8139

Merged
zhongjiajie merged 4 commits intoapache:devfrom
zhuangchong:dev-feature-dingtalk-add-at
Jan 21, 2022
Merged

[Feature-8138][Task] add at@ msg in the dingtalk task plugin#8139
zhongjiajie merged 4 commits intoapache:devfrom
zhuangchong:dev-feature-dingtalk-add-at

Conversation

@zhuangchong
Copy link
Copy Markdown
Contributor

@zhuangchong zhuangchong commented Jan 20, 2022

Purpose of the pull request

this pr close #8138

image

iShot2022-01-20 18 04 43

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:

zhongjiajie
zhongjiajie reviewed Jan 20, 2022
View reviewed changes
...main/java/org/apache/dolphinscheduler/plugin/alert/dingtalk/DingTalkAlertChannelFactory.java
Comment on lines +73 to +78
InputParam atUserIdsParam = InputParam
.newBuilder(DingTalkParamsConstants.NAME_DING_TALK_AT_USERIDS, DingTalkParamsConstants.DING_TALK_AT_USERIDS)
.addValidate(Validate.newBuilder()
.setRequired(false)
.build())
.build();
Copy link
Copy Markdown
Member

@zhongjiajie zhongjiajie Jan 20, 2022

Choose a reason for hiding this comment

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

Does mention users by id work for dingtalk now?

Copy link
Copy Markdown
Contributor Author

@zhuangchong zhuangchong Jan 21, 2022

Choose a reason for hiding this comment

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

The document provided by the official website has this field

https://open.dingtalk.com/document/robots/custom-robot-access

I tested the mobile phone number, but I didn't find out what the userId of DingTalk is, so I didn't test the userId

@zhongjiajie
Copy link
Copy Markdown
Member

zhongjiajie commented Jan 20, 2022

BTW, could you please add document to dingtalk alert ?

@zhuangchong
Copy link
Copy Markdown
Contributor Author

zhuangchong commented Jan 21, 2022

BTW, could you please add document to dingtalk alert ?

I'll update the DingTalk alert document right away.

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jan 21, 2022 •
edited
Loading

Codecov Report

Merging #8139 (445e249) into dev (a87ddca) will increase coverage by 0.04%.
The diff coverage is 91.66%.

Impacted file tree graph

@@             Coverage Diff              @@
##                dev    #8139      +/-   ##
============================================
+ Coverage     41.43%   41.47%   +0.04%     
+ Complexity     3735     3734       -1     
============================================
  Files           637      637              
  Lines         27141    27176      +35     
  Branches       3089     3092       +3     
============================================
+ Hits          11246    11272      +26     
- Misses        14811    14815       +4     
- Partials       1084     1089       +5
Impacted Files Coverage Δ
...plugin/alert/dingtalk/DingTalkParamsConstants.java 0.00% <ø> (ø)
...cheduler/plugin/alert/dingtalk/DingTalkSender.java 38.51% <78.57%> (+4.09%) ⬆️
...in/alert/dingtalk/DingTalkAlertChannelFactory.java 98.64% <100.00%> (+0.53%) ⬆️
...e/dolphinscheduler/remote/NettyRemotingClient.java 50.70% <0.00%> (-2.82%) ⬇️
...inscheduler/common/task/sqoop/SqoopParameters.java 74.00% <0.00%> (-2.00%) ⬇️
...api/service/impl/ProcessDefinitionServiceImpl.java 31.98% <0.00%> (-0.03%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a87ddca...445e249. Read the comment docs.

zhongjiajie
zhongjiajie previously approved these changes Jan 21, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@zhongjiajie zhongjiajie left a comment

Choose a reason for hiding this comment

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

LGTM, waiting CI

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud bot commented Jan 21, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

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

92.9% 92.9% Coverage
0.0% 0.0% Duplication

@zhongjiajie zhongjiajie added plug-in enhancement New feature or request miss:docs missing documents in PR labels Jan 21, 2022
zhongjiajie
zhongjiajie approved these changes Jan 21, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@zhongjiajie zhongjiajie left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

@zhongjiajie zhongjiajie merged commit 1fd748f into apache:dev Jan 21, 2022
@zhongjiajie
Copy link
Copy Markdown
Member

zhongjiajie commented Jan 21, 2022

Please add doc in the same level of https://dolphinscheduler.apache.org/en-us/docs/latest/user_doc/guide/alert/enterprise-wechat.html if you have time, thanks

@zhuangchong zhuangchong mentioned this pull request Jan 21, 2022
Merged
@zhuangchong zhuangchong deleted the dev-feature-dingtalk-add-at branch January 21, 2022 09:45
@zhongjiajie zhongjiajie removed the miss:docs missing documents in PR label Feb 15, 2022
@brave-lee brave-lee mentioned this pull request Feb 18, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zhongjiajie zhongjiajie zhongjiajie approved these changes

Assignees

@zhuangchong zhuangchong

Labels

enhancement New feature or request plug-in

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Feature][Task] add at@ msg in the dingtalk task plugin

3 participants

@zhuangchong @zhongjiajie @codecov-commenter