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

Email notification for flooded past-due actions #1083

Open
wants to merge 33 commits into
base: trunk
Choose a base branch
from

Conversation

crstauf
Copy link
Contributor

@crstauf crstauf commented Aug 5, 2024

Closes #1080.

I suspect there will be a lot of opinions and compatibility issues here, so providing only as a stepping stone for the Action Scheduler team, in an attempt to help them get it out quickly (we'll see).

@barryhughes barryhughes requested review from a team and jorgeatorres and removed request for a team August 13, 2024 23:03
@jorgeatorres jorgeatorres requested review from a team and Konamiman and removed request for jorgeatorres and a team August 30, 2024 23:49
Copy link
Contributor

@Konamiman Konamiman left a comment

Choose a reason for hiding this comment

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

Hi @crstauf, thanks for your contribution. I have a couple of comments:

  • The methods that are removed in classes/ActionScheduler_AdminView.php shouldn't actually be removed because they are public and the removal would break backwards compatibility. Could they be marked as deprecated, in the same way as they are in deprecated/ActionScheduler_AdminView_Deprecated.php below?
  • While receiving the notifications via email instead of as an admin notice seems indeed like it will be useful for most people, some might prefer to continue seeing these in the admin area. So maybe a setting could be provided to choose whether to use the new email notifications, the old warning in the admin area, or both.
  • Related to the above, a setting to provide an email address for the notifications (defaulting to the default admin email) would be useful.

@crstauf
Copy link
Contributor Author

crstauf commented Sep 4, 2024

Could they be marked as deprecated, in the same way as they are in deprecated/ActionScheduler_AdminView_Deprecated.php below?

@Konamiman I do not understand. I have defined the functions that were deprecated into the class that handles deprecated functions. Please advise.

some might prefer to continue seeing these in the admin area

This PR does not prevent the notice from appearing in the admin view. It seems you have not thoroughly reviewed or tested this PR?

Will add some appropriate filters.

@Konamiman
Copy link
Contributor

I have defined the functions that were deprecated into the class that handles deprecated functions.

Correct. I hadn't noticed that ActionScheduler_AdminView is actually extending ActionScheduler_AdminView_Deprecated.

It seems you have not thoroughly reviewed or tested this PR?

Indeed I haven't tested the PR yet, I was waiting to your answer to my comment before doing that.

Will add some appropriate filters.

Perfect, thank you. Please ping me once you do that and then I'll do all the testing.

@crstauf
Copy link
Contributor Author

crstauf commented Sep 14, 2024

@barryhughes Stalled.

Copy link
Contributor

@Konamiman Konamiman left a comment

Choose a reason for hiding this comment

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

Hi @crstauf, sorry for the delay in this second review. I have requested a few changes.

classes/ActionScheduler_PastDueMonitor.php Show resolved Hide resolved
classes/ActionScheduler_PastDueMonitor.php Show resolved Hide resolved
classes/ActionScheduler_PastDueMonitor.php Show resolved Hide resolved
classes/ActionScheduler_PastDueMonitor.php Show resolved Hide resolved
classes/ActionScheduler_PastDueMonitor.php Show resolved Hide resolved
@crstauf
Copy link
Contributor Author

crstauf commented Sep 17, 2024

Added documentation, did not make name change requests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants