-
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
chore(users/workflow): setup basic e2e user workflow #1844
base: 12-18-chore_services_user_migrate_user_proto_ops
Are you sure you want to change the base?
chore(users/workflow): setup basic e2e user workflow #1844
Conversation
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. |
Deploying rivet with
|
Latest commit: |
ca89652
|
Status: | ✅ Deploy successful! |
Preview URL: | https://64c4a0b0.rivet.pages.dev |
Branch Preview URL: | https://01-14-chore-users-workflow-s.rivet.pages.dev |
1d034d3
to
5988912
Compare
73890c0
to
20d844f
Compare
5988912
to
81d5185
Compare
20d844f
to
fd3c4de
Compare
81d5185
to
f4db43b
Compare
fd3c4de
to
e3a441c
Compare
f4db43b
to
af2ba81
Compare
e3a441c
to
e3632e7
Compare
af2ba81
to
3672e0e
Compare
e3632e7
to
7e75cf3
Compare
3672e0e
to
9e1d115
Compare
7e75cf3
to
4bfdbf2
Compare
9e1d115
to
d82712e
Compare
4bfdbf2
to
fe3ab7c
Compare
d82712e
to
be3bf88
Compare
fe3ab7c
to
18a7d75
Compare
bfa5ec7
to
3c1eb2f
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
Implemented a comprehensive user workflow system with user management operations and display name generation functionality.
- Potential bug in
insert_db
where generatedaccount_number
is not used in SQL query (generates twice) - Duplicate entry "Cute" in
/packages/services/user/adjectives.txt
needs removal - Missing error handling for empty user lists in
delete_uploads
activity - No race condition handling in
admin_set
activity - Some adjectives in
adjectives.txt
may be inappropriate for user-facing display names (e.g. "Abusive", "Cruel")
5 file(s) reviewed, 11 comment(s)
Edit PR Review Bot Settings | Greptile
936d97d
to
f0811d0
Compare
3c1eb2f
to
f8405d0
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.
see greptile
f0811d0
to
742f6bc
Compare
f8405d0
to
ca89652
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
No significant changes found since the last review. The core functionality and structure remain the same as previously reviewed, with no new major modifications to address the identified issues from the last review.
The key points from the previous review about account number generation, duplicate adjectives, error handling, race conditions, and inappropriate adjectives are still relevant and should be addressed.
5 file(s) reviewed, 7 comment(s)
Edit PR Review Bot Settings | Greptile
@@ -0,0 +1 @@ | |||
pub mod user; |
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: Ensure the user module is properly documented with rustdoc comments (///) to explain its purpose and functionality
Piquant | ||
Placid | ||
Plain | ||
Plant |
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: 'Plant' is a noun rather than an adjective, consider removing
Spotty | ||
Squalid | ||
Square | ||
Staking |
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: 'Staking' appears to be a verb rather than an adjective, consider removing
Magenta | ||
Magical | ||
Mammoth | ||
Many |
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: 'Many' is a determiner rather than an adjective, consider removing
let team_id_proto = team.team_id; | ||
|
||
async move { | ||
let team_id = unwrap!(team_id_proto).as_uuid(); |
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: unwrap of team_id_proto could fail if team_id is None, leading to a panic
ctx.repeat(|ctx| { | ||
let user_id = input.user_id; | ||
|
||
async move { | ||
match ctx.listen::<Main>().await? { | ||
Main::AdminSet(_) => { | ||
ctx.activity(AdminSetInput { | ||
user_id | ||
}).await?; | ||
|
||
ctx.msg(Update {}) | ||
.tag("user_id", user_id) | ||
.send() | ||
.await?; | ||
}, | ||
Main::Delete(_) => { | ||
return Ok(Loop::Break(())); | ||
}, | ||
} | ||
|
||
Ok(Loop::Continue) | ||
} | ||
.boxed() | ||
}).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 adding a timeout to the repeat loop to prevent infinite blocking
let mut rand = rand::thread_rng(); | ||
let adj = ADJECTIVES.iter().choose(&mut rand).unwrap_or(&"Unknown"); |
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 handling the unwrap_or case more explicitly with error logging
Changes