Skip to content

Validate notification fire date before scheduling #427

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.

Sign up for GitHub

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

Merged
ivan-magda merged 3 commits into from
Dec 12, 2018
Merged

Validate notification fire date before scheduling #427

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ab7618d
Validate fire date
ivan-magda Dec 10, 2018
78afc20
Up code quality
ivan-magda Dec 11, 2018
5f6ca17
Merge branch 'dev' into fix/expired-local-notifications-scheduling
ivan-magda Dec 12, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 31 additions & 0 deletions Stepic/Services/Notifications/LocalNotifications/LocalNotificationsService.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,19 @@ final class LocalNotificationsService {
throw Error.badContentProvider
}

let nextTriggerDate: Date?
Copy link
Contributor

@kvld kvld Dec 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Возможно, так понятнее

let nextTriggerDate: Date? = {
    switch notificationTrigger {
    case let timeIntervalTrigger as UNTimeIntervalNotificationTrigger:
        return timeIntervalTrigger.nextTriggerDate()
    case let calendarTrigger as UNCalendarNotificationTrigger:
        return calendarTrigger.nextTriggerDate()
    default:
        return nil
    }
}()

if let timeIntervalTrigger = notificationTrigger as? UNTimeIntervalNotificationTrigger {
nextTriggerDate = timeIntervalTrigger.nextTriggerDate()
} else if let calendarTrigger = notificationTrigger as? UNCalendarNotificationTrigger {
nextTriggerDate = calendarTrigger.nextTriggerDate()
} else {
nextTriggerDate = nil
}

if !self.isFireDateValid(nextTriggerDate) {
Copy link
Contributor

@kvld kvld Dec 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Аналогично, отрицание

throw Error.badFireDate
}

let request = UNNotificationRequest(
identifier: contentProvider.identifier,
content: self.makeNotificationContent(for: contentProvider),
Expand Down Expand Up @@ -186,6 +199,10 @@ final class LocalNotificationsService {
private func localNotificationScheduleNotification(
contentProvider: LocalNotificationContentProvider
) -> Promise<Void> {
if !self.isFireDateValid(contentProvider.fireDate) {
Copy link
Contributor

@kvld kvld Dec 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Без отрицания?

guard self.isFireDateValid(contentProvider.fireDate) else {
    return Promise(error: Error.badFireDate) 
}

Copy link
Contributor

@kvld kvld Dec 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Кстати, возможно, стоит прокомментировать, что localNotificationScheduleNotification возвращает промис, чтобы была совместимость с асинхронным методом в iOS 10.0+
Или написать тело так (кажется, так чуть понятнее):

return Promise { seal in
    guard self.isFireDateValid(contentProvider.fireDate) else {
        throw Error.badFireDate
    }

    // ...

    seal.fulfill(())
}

Copy link
Contributor

@kvld kvld Dec 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^ потому что приходится смотреть туда, где этот метод вызывается, так как непонятно, зачем там синхронный код обернут в промис

Copy link
Member Author

@ivan-magda ivan-magda Dec 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Да, для разных версий системы разное апи. Смотри как пытаются добиться единообразия https://stackoverflow.com/a/51052127 :trollface:

return Promise(error: Error.badFireDate)
}

let notification = UILocalNotification()
notification.alertTitle = contentProvider.title
notification.alertBody = contentProvider.body
Expand All @@ -202,6 +219,19 @@ final class LocalNotificationsService {
return .value(())
}

/// Checks that `fireDate` is valid.
///
/// - Parameters:
/// - fireDate: The Date object to be checked.
/// - Returns: `true` if the `fireDate` exists and it in the future, otherwise false.
private func isFireDateValid(_ fireDate: Date?) -> Bool {
if let fireDate = fireDate {
return fireDate.compare(Date()) == .orderedDescending
} else {
return false
}
}

// MARK: - Types -

enum PayloadKey: String {
Expand All @@ -212,5 +242,6 @@ final class LocalNotificationsService {

enum Error: Swift.Error {
case badContentProvider
case badFireDate
}
}