Skip to content
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.

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

Honor repeatInterval when rescheduling outdated notication #1294

Merged
merged 3 commits into from
Jul 20, 2018

Conversation

dluksza
Copy link
Contributor

@dluksza dluksza commented Jul 11, 2018

No description provided.

dluksza referenced this pull request Jul 11, 2018
[Android] Fix for setting past time in the scheduling.
newFireDate.add(Calendar.MINUTE, 1);
break;
case "hour":
newFireDate.add(Calendar.HOUR, 1);
Copy link
Member

@Salakar Salakar Jul 11, 2018

Choose a reason for hiding this comment

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

Would this not still have the same issue as before;

E.g.: I set a fireDate of 3 hours ago, it comes through this logic with a repeatInterval of an hour so this adds an hour but it would still be 2 hours behind - so fires immediately?

In the first instance setting a fireDate in the past shouldn't be allowed on the validations in the JS code - that stops one thing. But; when theRNFirebaseNotificationsRebootReceiver triggers rescheduleNotifications() after a device reboot there's a good chance fireDates can be in the past, so we'd still need to compensate for this anyway 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, it is not as easy as I initially expected. :|

@dluksza
Copy link
Contributor Author

dluksza commented Jul 16, 2018

@Salakar would you find time to review this again?

@ifsnow
Copy link
Contributor

ifsnow commented Jul 19, 2018

@Salakar It looks like there was a mistake in my previous PR. I think @dluksza's implementation is good. I'm really sorry to make you work twice.

@Salakar Salakar merged commit 408f998 into invertase:master Jul 20, 2018
@dluksza dluksza deleted the fix-past-date-scheduling branch July 20, 2018 11:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants