-
Notifications
You must be signed in to change notification settings - Fork 31
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
Fix method calls in DemographicRelationship #2
Conversation
Reviewer's Guide by SourceryThis PR fixes issues with the deletion flag handling in demographic relationships. The main changes involve removing an unnecessary setDeleted call during relationship creation and correcting the deletion flag setting when deleting relationships. The code also includes some structural improvements for better error handling. No diagrams generated as the changes look simple and do not need a visual representation. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Hey @sebastian-j-ibanez, here are examples of how you can ask me to improve this pull request: @Sweep Fix the CI errors. @Sweep Add unit tests for `DemographicRelationship.deleteDemographicRelationship()` that verify: 📖 For more information on how to use Sweep, please read our documentation. |
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.
Hey @sebastian-j-ibanez - I've reviewed your changes - here's some feedback:
Overall Comments:
- Please document why the change from Boolean.TRUE to ConversionUtils.toBoolString(Boolean.TRUE) was necessary. This seems like an important behavioral change that should be explained in the PR description.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Are these stepping on changes that were made in Magenta/OpenOSP main, @sebastian-j-ibanez ? |
41af51d
to
cd0aaaf
Compare
I changed the deleted field to match what Magenta has (should be a boolean field). It's good to go now. |
cd0aaaf
to
25cfc01
Compare
Had to adjust one more setDeleted call in MigrateRelationshipsToContactsHelper.java. It's trying to do a null check for the |
25cfc01
to
c24f87e
Compare
return deleted; | ||
} | ||
|
||
public void setDeleted(String deleted) { |
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.
That's a pretty big change going from a boolean to a string - seems intentional. Is it from an upstream commit? Feels like a rebase issue and changing the method isn't the solution maybe?
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.
The reason I made this change was because upstream was using a boolean.
This is what Magenta's main, alpaca, and bullfrog has (link):
public void setDeleted(boolean deleted) {
this.deleted = deleted;
}
Prevention Definition Update
Changes made
Summary by Sourcery
Fix calls to setDeleted in deleteDemographicRelationship and refactor the method for better readability.
Bug Fixes:
Enhancements: