Migrate the message poll to the generated PollResponseData model - #6680
Migrate the message poll to the generated PollResponseData model#6680gpunto wants to merge 3 commits into
Conversation
PR checklist ✅All required conditions are satisfied:
🎉 Great job! This PR is ready for review. |
SDK Size Comparison 📏
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review. WalkthroughThe message DTO now uses ChangesPoll mapping
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to Message poll parsing now uses the generated response model and its updated timestamp while preserving domain poll mapping and custom data behavior. The supplied coverage and current implementation alignment indicate no remaining merge-blocking risk. Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
andremion
left a comment
andremion
left a comment
There was a problem hiding this comment.
Looks good. One note inline on the nullability change and two optional ones.
| val moderation_details: DownstreamModerationDetailsDto? = null, // Used for Moderation V1 | ||
| val moderation: DownstreamModerationDto? = null, // Used for Moderation V2 | ||
| val poll: DownstreamPollDto? = null, | ||
| val poll: PollResponseData? = null, |
There was a problem hiding this comment.
voting_visibility goes from optional to required with this type. It was String? on DownstreamPollDto and it's String on PollResponseData, so a message whose poll omits the key used to default to PUBLIC and now the DTO path throws JsonDataException, which fails the whole message. The direct MessageAdapter still defaults it, so the two paths disagree on that input.
The backend always serializes the key, so it shouldn't fire in practice. Any reason not to note it in the Notes next to the created_at / updated_at / language bit? options and own_votes went the other way, lenient on the DTO path now and still strict in PollAdapter.
There was a problem hiding this comment.
Good catch, and confirmed: voting_visibility is required on PollResponseData while PollAdapter still defaults a missing key to PUBLIC. Noted in the Notes, along with options and own_votes swapping strictness the other way. All three are plain non-omitempty tags on the Go response struct, so none can fire in practice.
|
|
||
| val poll = with(sut) { response.toDomain() } | ||
|
|
||
| // `absent` is dropped: the domain map does not hold null values. |
There was a problem hiding this comment.
Small thing: this is a behaviour change for message polls too, not only a property of the new model. On develop a null-valued custom key stayed in Poll.extraData on the DTO path and got dropped on the direct path, so the two disagreed. mapNotNull fixes that.
Could be worth a line in the PR body so it reaches the release notes?
There was a problem hiding this comment.
Right, it changes the DTO path too. Added to the Notes: a null-valued custom key used to stay in Poll.extraData on the DTO path and was already dropped on the direct path, so this aligns them.
| } | ||
| ], | ||
| "created_by": {"id": "user-1", "role": "user", "banned": false, "online": true}, | ||
| "created_by": {"id": "user-1", "role": "user", "banned": false, "online": true, "created_at": "2020-01-01T00:00:00.000Z", "updated_at": "2020-01-01T00:00:00.000Z", "language": "en"}, |
There was a problem hiding this comment.
Could be nice: the poll here carries no custom keys, so assertBothPaths never compares poll or option extraData across the two parsers. PollResponseParsingTest covers the endpoint response but not the message.
Maybe add inlined custom keys on the poll and one option, like jsonWithMemberCustomInlined does for members? I tried it locally and both paths already agree, so it passes as is. Not blocking.
There was a problem hiding this comment.
Done in 4f390fa. The poll in jsonAllFields now carries an inlined custom key and option-1 carries one too, with a focused test asserting both land on the domain fields and option-2 stays empty. The poll also carries a null-valued key, so the same test pins the drop from the other comment.
|
2 participants
Goal
Parse the poll embedded in a message with the generated
PollResponseData.Part of AND-1291
Implementation
DownstreamMessageDto.pollatPollResponseData, the model the poll endpoints already use, and follow the field rename inlastUpdateTime().PollResponseData.toDomain()was already in place, so no mapping code is added.UserResponserequirescreated_at,updated_atandlanguage, and the fixture carried onlyid,role,bannedandonline.DownstreamPollDtostays for the poll events, which still parse it.Notes
The message carries
Poll *commonpayloads.PollResponseData, the same struct the poll endpoints return, andcreated_at,updated_atandlanguageare non-omitemptyonUserResponseCommonFields. So the wire always sends them for a nested poll user and the old fixture was the unrealistic part.The two paths swap strictness on three keys.
voting_visibilitywasString?and defaulted toPUBLIC; it is required onPollResponseData, so a poll that omits it now throws on the DTO path whilePollAdapterstill defaults it.optionsandown_votesgo the other way, defaulting to empty on the DTO path whilePollAdapterstill throws. None can fire in practice: all three are plain non-omitemptytags on the Go response struct.A custom key with a null value no longer reaches
Poll.extraData. The DTO path used to keep it and the direct path already dropped it, so this aligns them.Testing
getMessageandqueryChannels. Both custom values round-tripped, config was preserved, and every nested user (creator, votes, own votes, answers) carried the three required fields.message.poll, and droppingpoll.updatedAtfromlastUpdateTime, each fail 6 tests.MessageParsingTestgains a case for poll and option custom data inlined on the message, which also pins the null-valued key being dropped on both paths.Summary by CodeRabbit