CLAassistant
commented
1 month ago
All committers have signed the CLA.
Closed gusah009 closed 1 month ago
CLAassistant
commented
1 month ago
All committers have signed the CLA.
coderabbitai[bot]
commented
1 month ago [!WARNING]
Rate limit exceeded
@gusah009 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 11 minutes and 52 seconds before requesting another review.
How to resolve this issue?
After the wait time has elapsed, a review can be triggered using the `@coderabbitai review` command as a PR comment. Alternatively, push new commits to this PR. We recommend that you space out your commits to avoid hitting the rate limit.How do rate limits work?
CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our [FAQ](https://coderabbit.ai/docs/faq) for further information.Commits
Files that changed from the base of the PR and between ddfdff5bdc32db512b3dfba72f97558aac08cca6 and 6982a770a3faf09b5d02e4a4ecc802b46003baae.
This update enhances the AdminService by introducing functionalities for changing passwords, creating accounts, and deleting accounts. The changes include new API endpoints, RPC methods, and database operations, significantly improving administrative capabilities for managing accounts.
| File(s) | Change Summary |
|---|---|
api/docs/yorkie/v1/admin.openapi.yaml |
Added endpoints for changing passwords, creating accounts, and deleting accounts, with necessary schemas. |
api/yorkie/v1/admin.proto |
Introduced new RPC methods: CreateAccount, DeleteAccount, and ChangePassword with associated messages. |
server/backend/database/database.go |
Updated database methods to reflect account management changes, including errors and new functions for accounts. |
server/rpc/admin_server.go |
Added server methods for account deletion and password changes with appropriate validation. |
server/rpc/testcases/testcases.go |
Updated tests to reflect new account actions and added tests for deleting accounts and changing passwords. |
test/helper/helper.go |
Introduced a new variable for enhanced account creation password management. |
| Objective | Addressed | Explanation |
|---|---|---|
| Support basic account actions (e.g., ChangePassword, DeleteAccount) (#849) | ✅ |
🐇 In the meadow, with joy we bounce,
New powers for admin, we gladly announce!
Change a password, delete with ease,
User management now flows like a breeze.
Hoppy updates, oh what a delight,
For all the admin bunnies, it's a wondrous sight! 🌼
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?
krapie
commented
1 month ago @gusah009 There seems to be lint error causing CI to fail. Could you please lint it again?
gusah009
commented
1 month ago @krapie hello. I agree with all of your suggestions in the comments, so I've made the following changes.
In response to User vs Account, I have unified to use User. ex) DeleteAccount -> DeleteUser
I also agree that the Admin RPC needs test code, so I added a test case that is simple but covers a few cases.
In addition, I also included the review. Thank you for your careful review.
krapie
commented
1 month ago Also, I did not get what below comment means:
I wanted the validation of username or password on ChangePassword to be the same as SignUp, but I didn't know how to separate them into a common string, so I added a warning as a comment. If there's a good way to do it, I'd love to hear about it :)
gusah009
commented
1 month ago Also, I did not get what below comment means:
I wanted the validation of username or password on ChangePassword to be the same as SignUp, but I didn't know how to separate them into a common string, so I added a warning as a comment. If there's a good way to do it, I'd love to hear about it :)
@krapie
Well, this is not a very important comment because I merged ChangePasswordFields and SignUpFields into UserFields in this PR: https://github.com/yorkie-team/yorkie/pull/934#discussion_r1685435558
Anyway, that means, what it means is that I want to refactor the duplicated validation strings like "required,min=2,max=30,slug" or "required,min=8,max=30,alpha_num_special" because signup and changePassword must have the same validation. However, default strings in struct cannot be extracted as constants or anything in golang. That's all.
type SignupFields struct {
// Username is the name of user.
Username *string `bson:"username" validate:"required,min=2,max=30,slug"`
// Password is the password of user.
Password *string `bson:"password" validate:"required,min=8,max=30,alpha_num_special"`
}
type ChangePasswordFields struct {
// Username is the name of user.
Username *string `bson:"username" validate:"required,min=2,max=30,slug"`
// NewPassword is the new password of user
NewPassword *string `bson:"new_password" validate:"required,min=8,max=30,alpha_num_special"`
}@gusah009 How about changing the word user to account? If we use the word user in the context of admin, it sounds like the action is for normal user, not admin user. So if we use the word account instead, we can clarify the meaning; that these RPCs performs action on the admin itself. Currently Google is using the word account for their user system.
It will look something like below:
service AdminService {
rpc LogIn(LogInRequest) returns (LogInResponse) {}
rpc CreateAccount(CreateAccountRequest) returns (CreateAccountResponse) {}
rpc DeleteAccount(DeleteAccountRequest) returns (DeleteAccountResponse) {}
rpc ChangePassword(ChangePasswordRequest) returns (ChangePasswordResponse) {}
}@krapie Oh, that's a good idea. I'll do it right away.
Well, then I'll change the user table with admin to account table and accountName will be more appropriate than username.
krapie
commented
1 month ago @gusah009 Hmm. I'm not sure about changing username to accountName. I think we can organize like below:
Account is an object which contains Username, Password fields.Account parent of Admin and User (Normal User, TBD).gusah009
commented
1 month ago Account parent of Admin and User (Normal User, TBD).
@krapie Looking at it like this, I don’t think I need to change the username. Thank you for your good advice.
krapie
commented
1 month ago After discussing with @gusah009, we have concluded what we revert our changes to use RPC name as DeleteAccount and ChangePassword. This is because the scope of changing keyword User to Account takes more resources and changes that we thought. Considering our scope of this PR of just adding new actions for admin, it will be better to keep our changes small as possible.
I apologize for the frequent change request to @gusah009. Thank you for your understanding. Next time we will decide our design prior to implementation.
codecov[bot]
commented
1 month ago Attention: Patch coverage is 28.42105% with 68 lines in your changes missing coverage. Please review.
Project coverage is 51.31%. Comparing base (
bcb246b) to head (7022723).
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
krapie
commented
1 month ago @hackerwins We did not consider cascading delete on user related objects including projects and documents. I think we need to also consider to also delete related resources.
hackerwins
commented
1 month ago @krapie Please create an issue regarding this. 🙏
What this PR does / why we need it:
Added the admin APIs ChangePassword and DeleteAccount.
As their names suggest, these are APIs for changing passwords and deleting accounts.
Which issue(s) this PR fixes:
Fixes #849
Special notes for your reviewer:
I've verified that it works with mongoDB and in-memory DBs using postman.
As mentioned below, we will need to open an endpoint to access this API from your dashboard or CLI. client development is not included in this PR.
I generated the files with the
make protocommand, which broke the formatting of theapi/docs/yorkie/v1/*.openapi.yamlfiles.I wanted the validation of
usernameorpasswordonChangePasswordto be the same asSignUp, but I didn't know how to separate them into a common string, so I added a warning as a comment. If there's a good way to do it, I'd love to hear about it :)Does this PR introduce a user-facing change?:
There is no user impact yet. we need to open an endpoint that the client can access from the dashboard or CLI.
Additional documentation:
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation