-
Notifications
You must be signed in to change notification settings - Fork 82
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(user/workflows): profile set signal #1902
base: 01-14-chore_users_workflow_setup_basic_e2e_user_workflow
Are you sure you want to change the base?
feat(user/workflows): profile set signal #1902
Conversation
Deploying rivet with
|
Latest commit: |
277d41a
|
Status: | ✅ Deploy successful! |
Preview URL: | https://d2682d7f.rivet.pages.dev |
Branch Preview URL: | https://01-22-feat-user-workflows-pr.rivet.pages.dev |
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
How to use the Graphite Merge QueueAdd the label merge-queue to this PR to add it to the merge queue. You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
81d5185
to
f4db43b
Compare
c562f66
to
7c0e8aa
Compare
f4db43b
to
af2ba81
Compare
7c0e8aa
to
c70fe90
Compare
af2ba81
to
3672e0e
Compare
c70fe90
to
e62fbcd
Compare
3672e0e
to
9e1d115
Compare
e62fbcd
to
f129825
Compare
9e1d115
to
d82712e
Compare
f129825
to
af1c2e9
Compare
d82712e
to
be3bf88
Compare
af1c2e9
to
1854f2f
Compare
be3bf88
to
bfa5ec7
Compare
1854f2f
to
65dbb93
Compare
bfa5ec7
to
3c1eb2f
Compare
65dbb93
to
d1e1fb6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Summary
This PR implements a profile update workflow with improved error handling and status reporting. Here's a summary of the key changes:
- Added subscription-based profile update flow in
/packages/api/identity/src/route/identities.rs
usingProfileSetStatus
messages - Implemented
ProfileSet
signal handler in/packages/services/user/src/workflows/user/mod.rs
with validation and analytics tracking - Added dynamic query building for profile updates that only modifies specified fields
- Introduced proper error handling for validation failures and missing parameters
- Implemented cache invalidation after successful profile updates
The changes establish a more robust workflow for handling user profile updates with better status tracking and error reporting capabilities.
3 file(s) reviewed, 3 comment(s)
Edit PR Review Bot Settings | Greptile
3c1eb2f
to
f8405d0
Compare
d1e1fb6
to
fa080dd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
profile_set should not dynamically build queries
f8405d0
to
ca89652
Compare
fa080dd
to
277d41a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Summary
This PR updates the profile set functionality to use a more structured approach with SQL COALESCE. Here's what's new:
- Replaced dynamic query building with fixed COALESCE-based SQL update in
/packages/services/user/src/workflows/user/mod.rs
- Added proper SQL parameter handling for optional profile fields (display_name, account_number, bio)
- Maintained existing validation, error handling and analytics tracking from previous implementation
The changes follow the review feedback to avoid dynamic query building while preserving the workflow's core functionality.
3 file(s) reviewed, 4 comment(s)
Edit PR Review Bot Settings | Greptile
let mut sub = ctx | ||
.subscribe::<::user::workflows::user::ProfileSetStatus>(("user_id", user_ent.user_id)) | ||
.await?; | ||
|
||
ctx.signal(::user::workflows::user::ProfileSet { | ||
display_name: body.display_name.clone(), | ||
account_number: body.account_number.map(|n| n.api_try_into()).transpose()?, | ||
bio: body.bio.clone(), | ||
}) | ||
.tag("user_id", user_ent.user_id) | ||
.send() | ||
.await?; | ||
|
||
|
||
let status_msg = sub.next().await?; | ||
ensure!(status_msg.res.is_ok(), "bad profile set request"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: No timeout specified for subscription - could potentially hang indefinitely waiting for status message
|
||
|
||
let status_msg = sub.next().await?; | ||
ensure!(status_msg.res.is_ok(), "bad profile set request"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Generic error message 'bad profile set request' doesn't provide useful information about what failed
return Ok(Err(ProfileSetError::ValidationFailure)); | ||
} | ||
|
||
ctx.cache().purge("user", [input.user_id]).await?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: cache purge should happen after successful SQL update to prevent race conditions
if res.is_ok() { | ||
ctx.activity(PublishProfileSetAnalyticsInput { | ||
user_id | ||
}).await?; | ||
|
||
ctx.msg(Update {}) | ||
.tag("user_id", user_id) | ||
.send() | ||
.await?; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: consider moving Update message before analytics to ensure data consistency
Changes