fix: group description parsing, ephemeral trigger, participating roundtrip, business hour types - #488
Conversation
The IQ metadata parser was reading description text directly from the <description> node content, but WA Web wraps it in a <body> child: <description id="..." participant="..." t="..."><body>text</body></description> This caused description to always be None when fetching group metadata via IQ (sync_groups / get_metadata). The stanza notification parser already handled this correctly. Also adds description_owner and description_time fields that WA Web extracts (WAWebInGroupsGroupInfoDescriptionMixin), adds Node::content_as_string() helper to DRY the String/Bytes extraction pattern, and derives Default on GroupMetadata.
|
@claude review |
|
Claude finished @jlucaso1's task in 1m 6s —— View job Code Review: fix group description parsing
Looks good overall — clean fix for a real parsing bug, well-tested, and the Correctness ✅
Minor suggestions:
Style ✅
LGTM — ship it 🚢 |
Bencher Report
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdded optional description ownership/timestamp and ephemeral trigger fields to group metadata and group-info structs; updated parsing/serialization to read/write these attributes and to extract description text from nested Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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 |
- Parse `trigger` attribute from `<ephemeral>` node (WA Web extracts it as optional int 0-20 via InGroupsGroupInfoMixin) - Fix GroupParticipatingRequest::try_from_node() to actually read child presence instead of always returning default - Change BusinessHoursConfig open_time/close_time from Option<String> to u32 with default 0, matching WA Web's parseInt() behavior
jlucaso1
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wacore/src/iq/groups.rs`:
- Around line 2917-2927: Update the unit test
test_group_info_response_no_description to also assert that the newly added
fields are unset: after creating response via
GroupInfoResponse::try_from_node(&node), add assertions that
response.description_owner.is_none() and response.description_time.is_none() in
addition to the existing checks for description and description_id so
regressions on the new fields are caught.
- Around line 491-507: The emission guard currently only checks description and
description_id so description_owner and description_time are dropped; update the
outer condition to emit <description> whenever any of self.description,
self.description_id, self.description_owner, or self.description_time is Some
(e.g. if self.description.is_some() || self.description_id.is_some() ||
self.description_owner.is_some() || self.description_time.is_some()), then keep
building desc_builder (using NodeBuilder) and set attrs for id, participant
(description_owner) and t (description_time) and optional body (description) so
metadata-only descriptions serialize correctly and are pushed into children.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a7121859-2853-44c4-8e51-c70c71432208
📒 Files selected for processing (4)
src/features/groups.rswacore/binary/src/node.rswacore/src/iq/groups.rswacore/src/stanza/groups.rs
Emit <description> when any of its fields are set (not just text/id), and assert description_owner/description_time are None in the no-description test.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a2e5e16959
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| open_time: optional_attr(c, "open_time") | ||
| .and_then(|s| s.parse::<u32>().ok()) | ||
| .unwrap_or(0), |
There was a problem hiding this comment.
In BusinessProfileSpec::parse_response, missing or non-numeric open_time values are coerced to 0 via unwrap_or(0), and the struct now stores open_time/close_time as required u32. For modes like open_24h or appointment_only where these attributes may be omitted, this turns “not provided” into a real midnight value, so downstream consumers cannot distinguish absent data from an actual 00:00 schedule and will serialize misleading times.
Useful? React with 👍 / 👎.
2 participants
Summary
<description>content, but WA Web wraps it in<description><body>text</body></description>(verified viaWAWebInGroupsGroupInfoDescriptionMixin). Also added missingdescription_owneranddescription_timefields.<ephemeral expiration="X" trigger="Y">— we parsedexpirationbut missedtrigger(optional int 0-20, perInGroupsGroupInfoMixin).try_from_node()always returnedSelf::default(), ignoring whether<participants>and<description>children were present.open_time/close_timewereOption<String>but WA Web parses them as integers with default 0 (ParsersParseBusinessProfile).DRY improvements
Node::content_as_string()helper for the repeated String/Bytes extraction patternDefaultonGroupMetadata, used..Default::default()in testsTest plan
test_group_info_response_parses_description_from_body— description, id, owner, time from<body>childtest_group_info_response_no_description— None when no description nodecargo test --workspace --exclude e2e-tests)Summary by CodeRabbit
Release Notes
New Features
Improvements