[IDP-1221] Optionally send external-groups sync-errors notification email #375
Conversation
…-email # Conflicts: # application/features/groups-external-sync.feature
forevermatt
requested review from
briskt,
mtompset,
jason-jackson and
hobbitronics
and removed request for
a team
|
I'm not opposed to a breaking change, but would like to consider other things to include in the next major version. If this is a time-sensitive change, it might be good to do it in a non-breaking way and then circle back with the improved code. |
Good point. Changing the authenticate call to no longer update the user's last-login date/time will probably also be a breaking change, so it may be best to hold off on this minimum-version update until that is ready to release (and then perhaps raise our minimum version all the way to the oldest still-supported version of PHP). I'll look at adjusting this code back down to be compatible with PHP 7.4 |
|
Okay, I've lowered the minimum PHP version for ID Broker back to 7.4, to avoid the breaking change in this PR. |
My perspective would be that this is a bug fix, so it wouldn't be a breaking change. But it's a bit subjective. |
| <?= yHtml::ul($errors) ?> | ||
|
|
||
| <?php | ||
| if (empty($googleSheetUrl)) { |
There was a problem hiding this comment.
Isn't it impossible for googleSheetUrl to be empty? It would at least have this much: 'https://docs.google.com/spreadsheets/d/'
There was a problem hiding this comment.
Ahh, good catch. I'll try to fix that.
There was a problem hiding this comment.
Submitted follow-up PR to fix that:
#376
forevermatt
deleted the
feature/IDP-1221-sync-errors-notification-email
branch
IDP-1221 Send error notification email for external-groups sync errors
Added
local.env.distfor the environment variable name)Changed (non-breaking)
Emailerto send to an arbitrary email addressFixed
Feature PR Checklist
make composershowmake psr2