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.
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.
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.
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
Prevention Definition Update
…der names Addresses PR #225 review comment #2 (warrendennis): the previous PR removed Encode.forHtml(msg) at output because msg embedded intentional <span style='text-color: red;'>NOT</span> markup. The line 366 WARNING branch additionally interpolated raw provider first/last names into the msg via String.format, producing stored XSS against every admin who opens the page when a provider with HTML-in-name had no primary role. Split the previously-conflated msg into two surfaces: - msg (plain text) for the action result, output-encoded with an msgIsError flag driving alert-danger vs alert-info on the wrapper. - missingPrimaryRoleProviders (List<String> of raw names) rendered as a separate alert-warning <ul>, with each name output-encoded at the display site. Drop the pre-encoding helpers (encodedRoleNew, encodedRoleOld, forHtmlAttribute on number) — they were a workaround for the missing output encoding and were inconsistently applied (forHtmlAttribute on a HTML-body field, etc.). Use raw values for DAO/log/recycle bin paths where raw is required for correctness, and rely on output encoding at the display site.
Changes made
Summary by Sourcery
Fix calls to setDeleted in deleteDemographicRelationship and refactor the method for better readability.
Bug Fixes:
Enhancements: