This differential fixes invalid error handling in NSE code. Pure Objective-C @try{}...@catch{} statement cannot handle C++ std::exception. This differential solves this issue. Additionally StaffUtils::isStaffRelease functionality is used to
decide whether silence notification during error or display error message to the user in a notification.
Details
Details
- Put throw std:runtime_error("..."); just above [self decryptBestAttemptContent];
- Send Notification.
Before applying this diff nothing will be seen in NSE console. After applying this diff appropriate error message will be logged to the console and error notification displayed (assuming you changed StaffUtils::isStaffRelease to return true).
- Put @throw [NSException exceptionWithName:@"dummy" reason:nil userInfo:nil]; just above [self decryptBestAttemptContent];
- Send notification
Before applying this diff error will be logged to the console and notification silenced. After applying this diff appropriate error message will be logged to the console and error notification displayed (assuming you changed StaffUtils::isStaffRelease to return true).
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| native/ios/NotificationService/NotificationService.mm | ||
|---|---|---|
| 333 ↗ | (On Diff #30211) | Shouldn't we call this with bestAttemptContent? |
| native/ios/NotificationService/NotificationService.mm | ||
|---|---|---|
| 333 ↗ | (On Diff #30211) | It depends on where the error was caught. If the error was caught during decryption, then we have to silence notification, but if the error was caught later (e.x. notification persistence) and the notification can be displayed since it has user-readable content then we could call handler on the original content. But I would rather change implementation of how we handle errors that occur during persistence than this function. |
Comment Actions
Display correctly decrypted notification for a public user even if notification persistence fails
| native/ios/NotificationService/NotificationService.mm | ||
|---|---|---|
| 81 ↗ | (On Diff #30256) | In the last revision you modified this to include a check on comm::StaffUtils::isStaffRelease(). I'm a little confused by this change. It has several effects, including allowing the code to continue executing past line 89 for non-staff releases. Is this really intentional? I'm confused why we would have such a different behavior between staff and non-staff releases. Beyond that, I don't understand why we want to skip calling comm::Logger::log for non-staff releases. |
| native/ios/NotificationService/NotificationService.mm | ||
|---|---|---|
| 81 ↗ | (On Diff #30256) | On the previous version of this diff @tomek asked about callContentHandlerOnErrorMessage method. In particular he asked if we should rather call contentHandler on bestAttemptContent than silence notification for a non-staff user. I assumed that he thought we should display notification for a non-staff user even if error occurs since they are rather interested in getting notification instead of detecting a bug. If the notification decryption fails we cannot display anything to the user so we must silence notification (or deliberately let user see weird "ENCRYPTED" notification). However if notification persistence fails it means that we get to the point where we could persist notification so we must have correctly decrypted the notification so we have correct content to display to the user (or to attempt user-visible action such as rescind and badge update). This was the reason I introduced this change. |
| native/ios/NotificationService/NotificationService.mm | ||
|---|---|---|
| 81 ↗ | (On Diff #30256) | This explanation makes sense, but I'm not sure if it is a good idea to stop the execution for staff members but continue it for non-staff. We can consider e.g. continuing the execution as far as it makes sense and then either show a notif or error message containing all the error messages detected during the execution. Would that make sense? |
Comment Actions
- Complete early on notification decryption error only.
- Cumulate error messages from all stages of notification processing.
- Display cumulated error message for staff users and correctly decrypted content for public users
| native/ios/NotificationService/NotificationService.mm | ||
|---|---|---|
| 69 ↗ | (On Diff #30488) | Can we store errors in e.g. a vector and then join them by e.g. \n? |