Chrome extension author display name fix #29
Conversation
Fix git diff command
Tr/fix author -> tr/test
Tr/refresh token bug fix - testing on vibi-test
Revert debug logs and sign change, testing done
WalkthroughThe pull request introduces significant changes to improve the modularity and readability of the code. It modifies the handling of user and commit data by passing objects instead of keys or IDs, and refactors several functions for better error handling and clarity. The changes also include the addition of new fields to the Changes
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (6)
- vibi-dpu/src/bitbucket/reviewer.rs (1} hunks)
- vibi-dpu/src/bitbucket/user.rs (3} hunks)
- vibi-dpu/src/core/coverage.rs (2} hunks)
- vibi-dpu/src/utils/gitops.rs (6} hunks)
- vibi-dpu/src/utils/hunk.rs (3} hunks)
- vibi-dpu/src/utils/lineitem.rs (2} hunks)
Files skipped from review due to trivial changes (2)
- vibi-dpu/src/utils/gitops.rs
- vibi-dpu/src/utils/lineitem.rs
Additional comments: 10
vibi-dpu/src/bitbucket/user.rs (3)
6-6: The import of
User,Provider, andProviderEnumis new in this hunk. Ensure that these are used in the code that follows this hunk. Unused imports can lead to unnecessary code clutter.21-23: The function
author_from_commitis a new addition and replaces the oldget_commit_bbfunction. The new function returns anOption<BitbucketUser>instead ofOption<LineItem>. Ensure that this change is reflected in all places where this function is called.43-51: The error handling in this hunk is improved compared to the old hunk. The new code checks if the deserialization of the author is successful and returns
Noneif it fails. This is a good practice as it prevents the program from panicking at runtime due to unexpected data.vibi-dpu/src/bitbucket/reviewer.rs (2)
13-24: The function
add_reviewershas been refactored to accept aWorkspaceUserobject directly instead of auser_key. This change eliminates the need to fetch user data from the database using a key, which can improve performance. However, ensure that all calls to this function throughout the codebase have been updated to match the new signature.26-33: The function
add_user_to_reviewershas also been refactored to accept aWorkspaceUserobject directly. This change simplifies the function by removing the need to handle potential errors when fetching user data from the database. However, the function now assumes that theWorkspaceUserobject is valid and contains the necessary user data. Ensure that this assumption holds true in all cases where this function is called.vibi-dpu/src/utils/hunk.rs (3)
29-30: The addition of the
commitfield to theBlameItemstruct is a good move for storing commit information. However, the#[serde(skip_serializing)]attribute will prevent this field from being serialized. If this is the intended behavior, then it's fine. But if you want to serialize this field, you should remove this attribute.41-49: The
newmethod ofBlameItemhas been updated to include thecommitparameter. Ensure that all calls to this method throughout the codebase have been updated to match the new signature.74-76: The getter method for the
commitfield has been correctly implemented.---suggestion---
There is no setter method for thecommitfield. If you need to change thecommitfield after aBlameItemhas been created, you should add a setter method for it. Here's how you can do it:pub fn set_commit(&mut self, commit: String) { self.commit = commit; }vibi-dpu/src/core/coverage.rs (2)
3-3: The import of
user::BitbucketUseranduser::author_from_commithas been added. Ensure that these new imports are used in the code and are not left unused.28-55: The logic for adding reviewers has been significantly changed. Instead of directly using the author of the blame, the commit of the blame is used to get the author information using the
author_from_commitfunction. This author information is then used to get the workspace user from the database. If the workspace user is not found, the blame is skipped. If the workspace user is found, the first user in the users list of the workspace user is used as the actual blame user. The UUID of the actual blame user is then used to check if the user is already in the author set. If the user is not in the author set, the user is added to the author set and theadd_reviewersfunction is called with the workspace user.This change improves the accuracy of the reviewer assignment by ensuring that the author of the blame is a valid workspace user. However, it also introduces additional complexity and potential performance overhead due to the additional database query and the use of the
author_from_commitfunction. Consider optimizing this logic if performance becomes an issue.Also, the code assumes that the first user in the users list of the workspace user is the actual blame user. This may not always be the case. Consider improving the logic to accurately determine the actual blame user.
- add_reviewers(blame.author(), review, &access_token).await; + let blame_author_opt = author_from_commit(blame.commit(), + hunkmap.repo_name(), hunkmap.repo_owner()).await; + if blame_author_opt.is_none() { + eprintln!("[process_coverage] Unable to get blame author for commit: {}", &blame.commit()); + continue; + } + let blame_author = blame_author_opt.expect("Empty blame_author_opt"); + let user_key = blame_author.uuid().to_string(); + let blame_user_opt = get_workspace_user_from_db(&user_key); + if blame_user_opt.is_none() { + eprintln!("[process_coverage] no user found in db for blame author: {:?}", blame_author); + continue; + } + let blame_user = blame_user_opt.expect("Empty blame user"); + let blame_users: Vec<BitbucketUser> = blame_user.users().iter().cloned().collect(); + let actual_blame_user = blame_users[0].to_owned(); + let author_uuid = actual_blame_user.uuid(); + if author_set.contains(author_uuid) { + continue; + } + author_set.insert(author_uuid.to_string()); + add_reviewers(&blame_user, review, &access_token).await;
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- vibi-dpu/src/core/coverage.rs (2} hunks)
Additional comments: 1
vibi-dpu/src/core/coverage.rs (1)
- 3-3: The import statement has been updated to include new functions
author_from_commitandBitbucketUserstruct fromuser.rsanduser::BitbucketUserrespectively. This change is in line with the PR summary which mentions the introduction of a new functionauthor_from_commitand the use ofBitbucketUserstruct.
vibi-dpu/src/core/coverage.rs
Outdated
| add_comment(&comment, review, &access_token).await; | ||
| } | ||
| if repo_config.auto_assign() { | ||
| // add reviewers | ||
| println!("Adding reviewers..."); | ||
| let mut author_set: HashSet<String> = HashSet::new(); | ||
| author_set.insert(prhunk.author().to_string()); | ||
| for blame in prhunk.blamevec() { | ||
| if author_set.contains(blame.author()) { | ||
| let blame_author_opt = author_from_commit(blame.commit(), | ||
| hunkmap.repo_name(), hunkmap.repo_owner()).await; | ||
| if blame_author_opt.is_none() { | ||
| eprintln!("[process_coverage] Unable to get blame author for commit: {}", &blame.commit()); | ||
| continue; | ||
| } | ||
| author_set.insert(blame.author().to_string()); | ||
| let author_id = blame.author(); | ||
| add_reviewers(blame.author(), review, &access_token).await; | ||
| let blame_author = blame_author_opt.expect("Empty blame_author_opt"); | ||
| let user_key = blame_author.display_name().to_string(); | ||
| let blame_user_opt = get_workspace_user_from_db(&user_key); | ||
| if blame_user_opt.is_none() { | ||
| eprintln!("[process_coverage] no user found in db for blame author: {:?}", blame_author); | ||
| continue; | ||
| } | ||
| let blame_user = blame_user_opt.expect("Empty blame user"); | ||
| let bb_blame_user_opt = blame_user.users().iter().find(|user| user.uuid() == blame_author.uuid()); | ||
| let bb_blame_user: BitbucketUser; | ||
| if bb_blame_user_opt.is_none() { | ||
| let bb_users: Vec<BitbucketUser> = blame_user.users().iter().cloned().collect(); | ||
| let user_opt = bb_users.get(0); | ||
| if user_opt.is_none() { | ||
| eprintln!("[process_coverage] Empty Bitbucket Workspace User: {:?}", &blame_user); | ||
| continue; | ||
| } | ||
| bb_blame_user = user_opt.expect("Empty user_opt").to_owned(); | ||
| } | ||
| else { | ||
| bb_blame_user = bb_blame_user_opt.expect("Empty bb_blame_user_opt").to_owned(); | ||
| } | ||
| let author_uuid = bb_blame_user.uuid(); | ||
| if author_set.contains(author_uuid) { | ||
| continue; | ||
| } | ||
| author_set.insert(author_uuid.to_string()); | ||
| add_reviewers(&blame_user, review, &access_token).await; |
There was a problem hiding this comment.
The logic for adding reviewers has been significantly updated. Instead of directly using the author's ID from the blame, the new code fetches the author's information from the commit using the author_from_commit function. It then retrieves the workspace user from the database using the author's display name. If the workspace user is found, it checks if the user's UUID matches the blame author's UUID. If it doesn't, it uses the first user from the workspace user's users list. The UUID of the selected user is then added to the author_set and the user is added as a reviewer.
This change improves the accuracy of the reviewer assignment by ensuring that the correct user is added as a reviewer based on the blame information. However, it also introduces additional complexity and potential points of failure. For example, if the workspace user is not found in the database or if the user's users list is empty, the code will skip adding the user as a reviewer and print an error message. This could potentially lead to missing reviewers if the database is not up-to-date or if there are issues with the user data.
Consider adding a fallback mechanism to handle these cases, such as using the original author's ID if the workspace user is not found or if the users list is empty. This would ensure that a reviewer is always added, even if the optimal user cannot be determined.
+ if blame_user_opt.is_none() {
+ eprintln!("[process_coverage] no user found in db for blame author: {:?}", blame_author);
+ // Fallback to using the original author's ID
+ author_set.insert(blame_author.uuid().to_string());
+ continue;
+ }Committable suggestion (Beta)
| add_comment(&comment, review, &access_token).await; | |
| } | |
| if repo_config.auto_assign() { | |
| // add reviewers | |
| println!("Adding reviewers..."); | |
| let mut author_set: HashSet<String> = HashSet::new(); | |
| author_set.insert(prhunk.author().to_string()); | |
| for blame in prhunk.blamevec() { | |
| if author_set.contains(blame.author()) { | |
| let blame_author_opt = author_from_commit(blame.commit(), | |
| hunkmap.repo_name(), hunkmap.repo_owner()).await; | |
| if blame_author_opt.is_none() { | |
| eprintln!("[process_coverage] Unable to get blame author for commit: {}", &blame.commit()); | |
| continue; | |
| } | |
| author_set.insert(blame.author().to_string()); | |
| let author_id = blame.author(); | |
| add_reviewers(blame.author(), review, &access_token).await; | |
| let blame_author = blame_author_opt.expect("Empty blame_author_opt"); | |
| let user_key = blame_author.display_name().to_string(); | |
| let blame_user_opt = get_workspace_user_from_db(&user_key); | |
| if blame_user_opt.is_none() { | |
| eprintln!("[process_coverage] no user found in db for blame author: {:?}", blame_author); | |
| continue; | |
| } | |
| let blame_user = blame_user_opt.expect("Empty blame user"); | |
| let bb_blame_user_opt = blame_user.users().iter().find(|user| user.uuid() == blame_author.uuid()); | |
| let bb_blame_user: BitbucketUser; | |
| if bb_blame_user_opt.is_none() { | |
| let bb_users: Vec<BitbucketUser> = blame_user.users().iter().cloned().collect(); | |
| let user_opt = bb_users.get(0); | |
| if user_opt.is_none() { | |
| eprintln!("[process_coverage] Empty Bitbucket Workspace User: {:?}", &blame_user); | |
| continue; | |
| } | |
| bb_blame_user = user_opt.expect("Empty user_opt").to_owned(); | |
| } | |
| else { | |
| bb_blame_user = bb_blame_user_opt.expect("Empty bb_blame_user_opt").to_owned(); | |
| } | |
| let author_uuid = bb_blame_user.uuid(); | |
| if author_set.contains(author_uuid) { | |
| continue; | |
| } | |
| author_set.insert(author_uuid.to_string()); | |
| add_reviewers(&blame_user, review, &access_token).await; | |
| if blame_user_opt.is_none() { | |
| eprintln!("[process_coverage] no user found in db for blame author: {:?}", blame_author); | |
| // Fallback to using the original author's ID | |
| author_set.insert(blame_author.uuid().to_string()); | |
| continue; | |
| } |
vibi-dpu/src/core/coverage.rs
Outdated
| let blame_author_opt = author_from_commit(blame.commit(), | ||
| hunkmap.repo_name(), hunkmap.repo_owner()).await; | ||
| if blame_author_opt.is_none() { | ||
| eprintln!("[process_coverage] Unable to get blame author for commit: {}", &blame.commit()); | ||
| continue; | ||
| } | ||
| author_set.insert(blame.author().to_string()); | ||
| let author_id = blame.author(); | ||
| add_reviewers(blame.author(), review, &access_token).await; | ||
| let blame_author = blame_author_opt.expect("Empty blame_author_opt"); | ||
| let user_key = blame_author.display_name().to_string(); | ||
| let blame_user_opt = get_workspace_user_from_db(&user_key); | ||
| if blame_user_opt.is_none() { | ||
| eprintln!("[process_coverage] no user found in db for blame author: {:?}", blame_author); | ||
| continue; | ||
| } | ||
| let blame_user = blame_user_opt.expect("Empty blame user"); | ||
| let bb_blame_user_opt = blame_user.users().iter().find(|user| user.uuid() == blame_author.uuid()); | ||
| let bb_blame_user: BitbucketUser; | ||
| if bb_blame_user_opt.is_none() { | ||
| let bb_users: Vec<BitbucketUser> = blame_user.users().iter().cloned().collect(); | ||
| let user_opt = bb_users.get(0); | ||
| if user_opt.is_none() { | ||
| eprintln!("[process_coverage] Empty Bitbucket Workspace User: {:?}", &blame_user); | ||
| continue; | ||
| } | ||
| bb_blame_user = user_opt.expect("Empty user_opt").to_owned(); | ||
| } | ||
| else { | ||
| bb_blame_user = bb_blame_user_opt.expect("Empty bb_blame_user_opt").to_owned(); | ||
| } | ||
| let author_uuid = bb_blame_user.uuid(); |
There was a problem hiding this comment.
This whole thing should have been a separate function.
Variable rename comments handled in PR review.
…-dpu into tr/testAuthorFix
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (2)
- vibi-dpu/src/bitbucket/reviewer.rs (1} hunks)
- vibi-dpu/src/core/coverage.rs (2} hunks)
Files skipped from review due to trivial changes (1)
- vibi-dpu/src/bitbucket/reviewer.rs
Additional comments: 1
vibi-dpu/src/core/coverage.rs (1)
- 3-3: The import statement has been updated to include new modules and functions. The
author_from_commitfunction frombitbucket::userandBitbucketUserandWorkspaceUserfromutils::userare new additions. Ensure these new imports are used appropriately in the code.
bitbucket/reviewer: renamed multiple variables & functions to make more readable.
|
Kudos, SonarCloud Quality Gate passed!
|
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- vibi-dpu/src/bitbucket/reviewer.rs (6} hunks)
Additional comments: 5
vibi-dpu/src/bitbucket/reviewer.rs (5)
9-20: The function
add_reviewershas been refactored to take aBitbucketUserandReviewdirectly instead of fetching them from the database using a key. This change improves the function's modularity and makes it easier to test, as it no longer depends on the state of the database. However, ensure that theBitbucketUserandReviewpassed to this function are always valid and up-to-date.22-34: The function
get_updated_reviewers_vechas been refactored to take aBitbucketUserdirectly instead of fetching it from the database using a key. This change improves the function's modularity and makes it easier to test, as it no longer depends on the state of the database. However, ensure that theBitbucketUserpassed to this function is always valid and up-to-date.54-80: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [61-88]
The function
parse_reviewers_from_prinfohas been refactored to improve error handling. Instead of usingexpectto unwrap the result ofpr_info_response.json::<Value>().await, which could panic if the result is an error, it now checks if the result is an error and returnsNoneif it is. This change makes the function more robust and less likely to cause the program to crash.
- 109-115: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [91-113]
The function
put_reviewershas been slightly refactored. The change is minor and doesn't affect the function's behavior. The function still checks if theput_body_optisNoneand returns early if it is, preventing an unnecessary PUT request. The function then sends a PUT request to update the reviewers of a pull request and prints the response for debugging purposes.
- 131-137: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [134-144]
The function
prepare_get_prinfo_urlconstructs the URL for retrieving pull request information. The function has not been changed in this pull request.
Please check the action items covered in the PR -
Summary by CodeRabbit
BlameItemandLineItemstructures to store commit information, providing more comprehensive data for each item.