[iOSImportPasswordsSafari] File import policy block error [chromium/src : main]
Sourav Uttam Sinha (Gerrit)
Sourav Uttam Sinha added 2 comments![]( "Open in Gerrit")
if (canImportSomething) {
[self initializeDataSource];
[self.importStageTransitionHandler transitionToNextImportStage];
} else if (anyTypeBlockedByPolicy) {
[self.importStageTransitionHandler
resetToInitialImportStage:DataImportResetReasonAllDataBlockedByPolicy];
} else {
[self.importStageTransitionHandler
resetToInitialImportStage:DataImportResetReasonNoImportableData];
}@tmar...@chromium.org let me know if I should add comments or is it self-explanatory.
alert.title = @"Import Not Allowed";
alert.message =
@"Importing this data is not allowed by your organization's policy.";
shouldRecordFailure = YES;@tmar...@chromium.org @ginny...@chromium.org strings will be localized once finalized.
Related details
- Ginny Huang
- Tommy Martino
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ginny Huang (Gerrit)
Ginny Huang voted and added 6 comments![]( "Open in Gerrit")
Votes added by Ginny Huang
| Code-Review | +1 |
6 comments
Logic lgtm, but please take care of the readability nits if you aren't in a hurry of submission :)
BOOL canImportSomething = NO;Readability nit: `shouldInitiateImport`. Having "something" in a variable name makes it feel like pseudocode 😂
canImportSomething = YES;Nit: add a `break` after this line. It seems that we only care about `blockedByPolicy` when this is false...
- (UIAlertController*)errorAlert {I encourage you to remove the `errorAlert` property and turn this into a helper method that takes a `title` and a `message` instead 😊
The reason why it was originally a variable was because I thought I needed to call `weakSelf.errorAlert` in the dismiss handler. Turns out that it's not needed, which is why line 181 is now `handler:nil`.
Thanks!
SafariDataImportStage currentStage = _containerViewController.importStage;`self.importStage` would return exactly the same thing. Why are you changing this part?
alert.title = @"Import Not Allowed";If you are planning to land like this, add a TODO on the top showing that you are planning to replace this part and use L10n instead.
Related details
- Sourav Uttam Sinha
- Tommy Martino
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sourav Uttam Sinha (Gerrit)
Sourav Uttam Sinha added 5 comments![]( "Open in Gerrit")
Readability nit: `shouldInitiateImport`. Having "something" in a variable name makes it feel like pseudocode 😂
Done
Nit: add a `break` after this line. It seems that we only care about `blockedByPolicy` when this is false...
Done
I encourage you to remove the `errorAlert` property and turn this into a helper method that takes a `title` and a `message` instead 😊
The reason why it was originally a variable was because I thought I needed to call `weakSelf.errorAlert` in the dismiss handler. Turns out that it's not needed, which is why line 181 is now `handler:nil`.
Thanks!
Done
SafariDataImportStage currentStage = _containerViewController.importStage;`self.importStage` would return exactly the same thing. Why are you changing this part?
Correct! Changed it back
If you are planning to land like this, add a TODO on the top showing that you are planning to replace this part and use L10n instead.
I will be landing this CL once the strings are localized.
Related details
- Ginny Huang
- Tommy Martino
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ginny Huang (Gerrit)
Ginny Huang added 4 comments![]( "Open in Gerrit")
/// Alert screen being displayed when the last selected file could not be
/// processed or contains no valid items.My apologies this would be my fault... but would you mind updating this comment:
/// Alert screen being displayed before dismissal, asking the user whether the imported file should be deleted.
BOOL shouldRecordFailure = NO;Do you really need this variable? Because it's always the same as `alert != nil` and so you can just keep checking the latter :)
BOOL success = [self presentViewController:alert];Don't capture `self` in a block :)
if (_containerViewController.presentedViewController &&
!_containerViewController.presentedViewController.isBeingDismissed) {
[_containerViewController
dismissViewControllerAnimated:NO
completion:presentAlertBlock];
} else {This check does not exist previously. Can we remove it? If you think it's necessary we can add it in a follow up CL.
Related details
- Sourav Uttam Sinha
- Tommy Martino
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ginny Huang (Gerrit)
Ginny Huang added 1 comment![]( "Open in Gerrit")
BOOL success = [self presentViewController:alert];Don't capture `self` in a block :)
I'll lgtm once this is fixed
Tommy Martino (Gerrit)
Tommy Martino added 3 comments![]( "Open in Gerrit")
typedef NS_ENUM(NSInteger, DataImportResetReason) {The Best Practices document prefers the use of `enum class` over `NS_ENUM`:
go/bling-best-practices#ns-enum-vs-c-enums
if (canImportSomething) {
[self initializeDataSource];
[self.importStageTransitionHandler transitionToNextImportStage];
} else if (anyTypeBlockedByPolicy) {
[self.importStageTransitionHandler
resetToInitialImportStage:DataImportResetReasonAllDataBlockedByPolicy];
} else {
[self.importStageTransitionHandler
resetToInitialImportStage:DataImportResetReasonNoImportableData];
}@tmar...@chromium.org let me know if I should add comments or is it self-explanatory.
lgtm
Related details
- Sourav Uttam Sinha
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sourav Uttam Sinha (Gerrit)
Sourav Uttam Sinha added 5 comments![]( "Open in Gerrit")
typedef NS_ENUM(NSInteger, DataImportResetReason) {The Best Practices document prefers the use of `enum class` over `NS_ENUM`:
go/bling-best-practices#ns-enum-vs-c-enums
Done
/// Alert screen being displayed when the last selected file could not be
/// processed or contains no valid items.My apologies this would be my fault... but would you mind updating this comment:
/// Alert screen being displayed before dismissal, asking the user whether the imported file should be deleted.
Done
Do you really need this variable? Because it's always the same as `alert != nil` and so you can just keep checking the latter :)
Thanks for the suggestion!
Ginny HuangDon't capture `self` in a block :)
I'll lgtm once this is fixed
Done
if (_containerViewController.presentedViewController &&
!_containerViewController.presentedViewController.isBeingDismissed) {
[_containerViewController
dismissViewControllerAnimated:NO
completion:presentAlertBlock];
} else {This check does not exist previously. Can we remove it? If you think it's necessary we can add it in a follow up CL.
- Ginny Huang
- Tommy Martino
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ginny Huang (Gerrit)
Ginny Huang voted and added 1 comment![]( "Open in Gerrit")
Votes added by Ginny Huang
| Code-Review | +1 |
1 comment
Import Not AllowedPlease double check that "Not Allowed" should be capitalized. IIRC our UXW team is moving away from title case, so maybe it should be "Import not allowed"
Related details
- Sourav Uttam Sinha
- Tommy Martino
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sourav Uttam Sinha (Gerrit)
Sourav Uttam Sinha added 1 comment![]( "Open in Gerrit")
Please double check that "Not Allowed" should be capitalized. IIRC our UXW team is moving away from title case, so maybe it should be "Import not allowed"
- Ginny Huang
- Tommy Martino
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ginny Huang (Gerrit)
Ginny Huang voted Code-Review+1![]( "Open in Gerrit")
- Sourav Uttam Sinha
- Tommy Martino
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tommy Martino (Gerrit)
Tommy Martino voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Sourav Uttam Sinha
- Tommy Martino
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tommy Martino (Gerrit)
Tommy Martino voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Sourav Uttam Sinha
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sourav Uttam Sinha (Gerrit)
Sourav Uttam Sinha voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[iOSImportPasswordsSafari] File import policy block error
Show a specific error message if a user tries to import a file, and all
the data types found within that file are blocked by enterprise policy.
This handles the case where the import option is available (because not
all 4 data types are blocked), but the specific file's contents are
fully restricted by policy.
- M ios/chrome/app/strings/ios_strings.grd
- A ios/chrome/app/strings/ios_strings_grd/IDS_IOS_SAFARI_IMPORT_IMPORT_NOT_ALLOWED_MESSAGE.png.sha1
- A ios/chrome/app/strings/ios_strings_grd/IDS_IOS_SAFARI_IMPORT_IMPORT_NOT_ALLOWED_TITLE.png.sha1
- M ios/chrome/browser/data_import/ui/data_import_import_stage_transition_handler.h
- M ios/chrome/browser/data_import/ui/import_data_item_table_view.mm
- M ios/chrome/browser/safari_data_import/coordinator/safari_data_import_import_coordinator.mm
- M ios/chrome/browser/safari_data_import/coordinator/safari_data_import_import_mediator.mm
Code-Review: +1 by Tommy Martino, +1 by Ginny Huang
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |