[PM-41028] feat: Restrict View Send screen for policy non-compliant Sends - #2920
Conversation
github-actions
Bot
added
app:password-manager
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE Reviewed the View Send restriction banner and its state derivation ( Code Review DetailsNo new findings at or above the reporting threshold. Prior inline threads are either resolved or awaiting a team discussion on test naming, which is left to the humans on the thread. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## matt/PM-38792-send-controls-restrict-deletion-days #2920 +/- ##
======================================================================================
+ Coverage 79.60% 79.62% +0.01%
======================================================================================
Files 1169 1169
Lines 75261 75336 +75
======================================================================================
+ Hits 59913 59984 +71
- Misses 15348 15352 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
15181ef to
0603468
Compare
fedemkr
left a comment
fedemkr
left a comment
There was a problem hiding this comment.
Looks good, just some minor comments.
| mode: .add, | ||
| name: sendView.name, | ||
| notes: sendView.notes ?? "", | ||
| recipientEmails: sendView.emails.isEmpty ? [""] : sendView.emails, |
There was a problem hiding this comment.
🤔 Is it needed to add an empty string as one of the emails recipient when the sendView.emails is empty? If that's actually necessary, then a test is missing for that case.
There was a problem hiding this comment.
It's needed to provide an empty text field for the user to add an email. I'll add a test.
| /// `isDisabled`, `restrictionBannerMessage`, and `canMakeCopy` reflect a non-disabled Send. | ||
| func test_isDisabled_notDisabled() { | ||
| let subject = ViewSendItemState(sendView: .fixture(disabled: false)) | ||
| XCTAssertFalse(subject.isDisabled) | ||
| XCTAssertNil(subject.restrictionBannerMessage) | ||
| XCTAssertFalse(subject.canMakeCopy) | ||
| } | ||
|
|
||
| /// A disabled File Send shows the generic non-compliant message and offers no copy, regardless of | ||
| /// whether its type is restricted by policy. | ||
| func test_isDisabled_disabledFileSend() { | ||
| let subject = ViewSendItemState(sendView: .fixture(type: .file, disabled: true)) | ||
|
|
||
| XCTAssertTrue(subject.isDisabled) | ||
| XCTAssertEqual(subject.restrictionBannerMessage, Localizations.thisSendIsNotCompliantDescriptionLong) | ||
| XCTAssertFalse(subject.canMakeCopy) | ||
| } | ||
|
|
||
| /// A disabled Text Send whose type is restricted by policy shows the text-restricted message and | ||
| /// offers no copy. | ||
| func test_isDisabled_disabledTextSend_typeRestricted() { | ||
| var subject = ViewSendItemState(sendView: .fixture(type: .text, disabled: true)) | ||
| subject.sendPolicyOptions = SendPolicyOptions(enforcedSendType: .file) | ||
|
|
||
| XCTAssertTrue(subject.isDisabled) | ||
| XCTAssertEqual(subject.restrictionBannerMessage, Localizations.textSendsAreNotAllowedDescriptionLong) | ||
| XCTAssertFalse(subject.canMakeCopy) | ||
| } | ||
|
|
||
| /// A disabled Text Send whose type isn't restricted by policy offers the "Make a copy" action. | ||
| func test_isDisabled_disabledTextSend_typeNotRestricted() { | ||
| let subject = ViewSendItemState(sendView: .fixture(type: .text, disabled: true)) | ||
|
|
||
| XCTAssertTrue(subject.isDisabled) | ||
| XCTAssertEqual(subject.restrictionBannerMessage, Localizations.toEditThisSendMakeACopyDescriptionLong) | ||
| XCTAssertTrue(subject.canMakeCopy) | ||
| } | ||
|
|
There was a problem hiding this comment.
🤔 This is a different approach as how we normally do tests. It's a bit confusing to have the test saying isDisabled when that's not the actual focus of the test and at least you would just need 2 instead of 4 test functions to test that particular property.
I understand this is to save doing a lot of tests to test the functions separately but wanted to chat a bit on whether we'd like to deviate from our standard into this kind of several-in-one tests which saves several lines of code. cc: @bitwarden/team-ios
If so, perhaps we'd need to update a bit the naming convention for these cases and/or the properties to be tested in the test functions' docs.
There was a problem hiding this comment.
Good point. It seemed worthwhile to combine these tests given that these properties are all tightly coupled. The common theme here is testing the properties related to disabled sends due to type restrictions. What if we rename them to test_restrictionState_xyz?
0603468 to
c5bb798
Compare
…t just if policy requires it
766b9c4 to
0bbd066
Compare
2 participants
🎟️ Tracking
PM-41028
📔 Objective
SendPolicyOptions.isSendTypeRestricted(for:)to determine whether an existing Send violates the active Send Controls policy (restricted type takes priority over restricted access type/deletion date/hide-email).ActionCard"Organization policy restriction" banner for non-compliant Sends, with copy varying by violation: restricted text type, restricted file type, or restricted fields (with a "Make a copy" action).AddEditSendItemProcessorpolicy-enforcement logic.📸 Screenshots