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

feat: 新增 微信客服 - 会话分配与消息收发 fix: 模板卡片参数非必填 若不修改会导致接口报错 #189

Merged
merged 2 commits into from
Jan 24, 2024

Conversation

tttao7
Copy link

@tttao7 tttao7 commented Jan 9, 2024

feat: 新增 微信客服 - 会话分配与消息收发 获取会话状态 & 变更会话状态

fix: 模板卡片参数非必填 若不修改会导致接口报错
目前测试下来 当不需要 ActionMenu 时 会发生报错 原因是因为json序列化会默认把默认值放入
@xen0n @eryajf 如果有更好的方案 请指教

@tttao7 tttao7 changed the title fix: 模板卡片参数非必填 若不修改会导致接口报错 feat: 新增 微信客服 - 会话分配与消息收发 fix: 模板卡片参数非必填 若不修改会导致接口报错 Jan 24, 2024
xen0n
xen0n previously approved these changes Jan 24, 2024
Copy link
Owner

@xen0n xen0n left a comment

Choose a reason for hiding this comment

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

Thanks!

@xen0n xen0n dismissed their stale review January 24, 2024 11:08

Wait a moment

@@ -1289,26 +1289,26 @@ const (
type TemplateCard struct {
CardType TemplateCardType `json:"card_type"`
Source Source `json:"source"`
ActionMenu ActionMenu `json:"action_menu,omitempty" validate:"required_with=TaskID"`
ActionMenu *ActionMenu `json:"action_menu,omitempty" validate:"required_with=TaskID"`
Copy link
Owner

Choose a reason for hiding this comment

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

刚发现目前的改法变动了公开类型定义,这应该会导致明确用到了相关字段的下游代码编译错误。

为了不破坏 v1.x 的源码兼容性,可能只能保持这边不动,然后弄一个私有的镜像类型(可以简单命名叫 templateCard 或者别的),在那边把这些字段的类型改了。然后需要在库代码里双向转换:从写作 T 的面向用户的类型转为写作 *T 的内部类型时,先把它跟 T{} 比较,如果是零值的话,那么就保持指针类型的字段为 nil,否则填一个指针;反向操作也差不多。

这样如何?

Copy link
Author

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.

详见 #177

Copy link
Owner

Choose a reason for hiding this comment

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

哦哦,意思是无论如何,master 分支的用户都必须改是吧?那就不算 API-breaking 了 ;-)

确认下是这样的话我就合并了

Copy link
Author

Choose a reason for hiding this comment

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

是的 经过测试 如果不传这个值 企业微信那边会报错 xx参数必填 但是实际情况他是选填的 而这个结构体又是刚合并没多久的 或者可以等写这个的小伙伴 @eryajf 回复 json序列化把零值序列进去了

Copy link
Owner

Choose a reason for hiding this comment

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

或者可以等写这个的小伙伴 @eryajf 回复

OK,我等他半天左右(大概明天中午),否则鉴于影响面也小,我就直接合并 ;-)

Copy link
Contributor

Choose a reason for hiding this comment

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

我目前没有这块儿的验证场景,所以请根据你们的判断对pr进行处理即可。感谢等待

@xen0n xen0n merged commit 2fe83ea into xen0n:develop Jan 24, 2024
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