fix: deliver the configured mentions in every notification mode - #404
Merged
Conversation
Which half of a mention has to be supplied depends on the message type. For markdown and
actionCard the body has to carry the `@<mobile>` token or the mobile is never resolved, and
actionCard needs one for `isAtAll` too; for text DingTalk renders the mention from the `at`
object itself, and adding one to the body only gets it rendered twice; the link type supports
no mention at all. Six places got that wrong:
* the raw ("disable built-in message") branch built its MessageModel without atAll or
atMobiles, so disabling the built-in message silently dropped every mention;
* actionCard, which the built-in notification uses, needs a token in the body for
`isAtAll` too, and none was ever written — so "notify everyone" only worked by accident,
when the custom content happened to contain some other `@`;
* the mention was appended unconditionally, so a template that already mentioned someone
got a second copy of the same mention, which DingTalk renders twice;
* mobiles were appended even when everyone was being mentioned, but DingTalk stops
resolving individual mobiles in that case, leaving bare phone numbers in the message
body where a name should have appeared — and notifying nobody who was not already
covered by mentioning everyone;
* text messages had a mention appended as well, which DingTalk renders in addition to the
one it adds itself;
* link messages, which DingTalk documents as not supporting mentions at all, were given
both the at object and an appended `@<mobile>` that can only ever look like a mention.
The mention fields also lived inside the block the UI hides when the built-in message is
disabled, so there was no way to configure them in the mode that now honours them.
`At.isAtAll` becomes a primitive so that reading it needs no null handling; it is a
request object that is never persisted, and Gson keys off the field name, so the wire
format is unchanged.
A mention in a markdown or actionCard message is not tappable on the mobile client — that is
DingTalk's own documented behaviour, not something this change can fix — so the docs now say
so where people configure mentions: a new "@ 人" page, the field's own help text, and the two
pages that used to promise a mention without qualifying it.
Users who ticked "notify everyone" will see an `@所有人` line where they saw none before,
will no longer see the phone numbers that used to be appended next to it, and on the
built-in notification it now notifies reliably rather than occasionally. A text message's
mention moves onto its own line, which is where DingTalk puts the one it renders.
Signed-off-by: BobDu <i@bobdu.cc>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #279
Fixes #305
Also addresses item 2 of #282 (see below), and re-opens the question closed by #275.
The mechanism, because the bugs only make sense against it
A DingTalk mention has two halves: the
atobject decides who gets notified, and a@<mobile>token in the message body decides where the mention is rendered. Which half youhave to supply depends on the message type, and the plugin treated them as if it did not:
markdown/actionCardisAtAlltoo.textatobject itself, and appends its own copy even when the body already has one.linkSix defects on that path
The raw ("disable built-in message") branch never sent
at. ItsMessageModelwas builtwithout
atAll/atMobilessince raw mode was introduced inc66a0df, so ticking "disablebuilt-in message" silently dropped every mention — 禁用内置消息 希望可以有@别人的功能 #279, and item 2 of 希望markdown支持模板语法 #282.
"Notify everyone" did nothing on the built-in notification. The built-in message is an
actionCard, which needs an
@所有人token in the body forisAtAllto take effect, and none wasever written. It only worked by accident, when the custom content happened to contain some other
@— which is why it behaved intermittently and was never reported on its own.The mention was appended unconditionally, so a template that already mentioned someone got a
second copy — DingTalk renders both. That is 这个插件发起的通知里面可以把 @被通知人 插入消息中间,而不是结尾吗? #305.
Mobiles were appended even when everyone was being mentioned. DingTalk stops resolving
individual mobiles once
isAtAllis set, so those tokens stayed unresolved: a bare phone numbersitting in the group message where a name should have been, notifying nobody who was not already
covered.
Text messages got a mention appended as well, which DingTalk renders in addition to the one
it adds itself.
Link messages were given both the
atobject and an appended@<mobile>. Neither doesanything for that type — the token is never resolved, so it only left a bare phone number behind.
The mention fields also lived inside the block the UI hides when the built-in message is disabled,
so there was no way to configure them in the mode that now honours them. They move out; the
JavaScript that hides the block is unchanged.
At.isAtAllbecomes a primitive so reading it needs no null handling. It is a request object that isnever persisted, and Gson keys off the field name, so the wire format is unchanged.
What changes for users
@所有人line where there was none, and on thebuilt-in notification it notifies reliably instead of occasionally.
in the message. They were never resolved into names and never notified anyone extra.
visible in that mode.
Documentation
Mentions in
markdownandactionCardmessages are not tappable on the DingTalk mobile client.DingTalk documents the rendering half of this — "机器人发送 Markdown 消息类型@人员,不支持高亮显示",
with a per-type table marking Text 是 / Markdown 否 / ActionCard 否
(robot-message-type-staff-information-in-an-enterprise) —
and testing against a live robot confirms the mention is also not tappable there, while a
TEXTmessage's mention is tappable on both clients. Using
atUserIdsinstead of a mobile does not changethis: the message type is what decides it, not the identifier.
That is almost certainly what #218 reports. It cannot be fixed inside an actionCard, so this PR
documents it instead of changing behaviour: a new @ 人 page covers how mentions work and what
each message type can do, and the field's own help text plus the two pages that used to promise a
mention without qualifying it now point at it.
Testing done
mvn clean verifypasses: 67 tests, plus spotless, access-modifier-checker, the enforcer importrules and spotbugs (0 findings).
The new tests assert on the JSON actually posted, by pointing the robot's webhook at a local
HttpServer— the first payload-level coverage in this repository:DingTalkSenderAtTestcovers the token rules — appending, not repeating a token the body alreadyhas, leaving mobiles out when everyone is mentioned, adding nothing to text or link messages.
DingTalkRunListenerAtTestruns a real build throughJenkinsRulefor both notification modes andchecks the payload carries the configured mention.
Each of the six fixes was checked by mutation: reverting it on its own makes the corresponding test
fail, so the tests reproduce the defects rather than merely exercising the lines.
The behaviour the fixes rely on was established against a live robot rather than inferred — which
message types resolve a mention, whether a token is required, what happens when
isAtAllandatMobilesare combined, and whether wrapping the token in<font>matters (it does not).Related issues
@ALLafter disabling the built-inmessage). Item 1 (template syntax in markdown) already works via environment-variable expansion —
worth confirming with the reporter. Item 3 (deciding whether to notify based on an environment
variable) is not implemented and deserves its own issue, so 希望markdown支持模板语法 #282 is deliberately not auto-closed.
@手机号into the custom message. That could not work,because the raw branch never sent
atMobiles— it does now.Submitter checklist