Mail user on TransactionStop and SuspendedEV event

[fnkbsi]

Send a email to the transaction user in case of SuspendedEV and Transaction Stop, if the email address of the user is stored in the database.

[Zeppel]

Steve tries infinitely to send mails for every tag without a user. After some time the server becomes unstable.

[fnkbsi]

@Zeppel: Thanks for the feedback. I try to reproduce the behavior. It would help if you can answer some the following questions. On which event starts SteVe to try sending mails? Which notification are selected on the setting-website? Is the List of recipients empty? Are the OCPP-Tags without user blocked or active? (Does SteVe try to send to both?) Is the transaction started with an OCPP-Tag without a user? Can you provide a log file?

[Zeppel]

@fnkbsi Event: occures at SuspendedEV event

Selected Notifies:

1 entry in recipients

OCPP-Tag is active and started the transcation without an user.

Log attached - Exception occures 10x per second until a user is found, no mails will be send. If a user is selected for the tag afterwards, mails are send to the user and recipient email. Errors stop then. steve.log

occurred location: src/main/java/de/rwth/idsg/steve/repository/UserRepository.java User.Details getDetails

Expected behavior: return and do nothing

[fnkbsi]

@Zeppel: My hypothesis is, the exception blocks sending the response to transactionStop, so the charge box resend the transactionStop message and that leads to a loop. By catching the exception and calling the notification asynchronously it should now work. Thanks again for the feedback!

[juherr]

@goekay Please review, this PR is improving the product

[goekay]

what if the user does not want to receive emails?

[fnkbsi]

what if the user does not want to receive emails?

Currently the mail-address must removed. I admit this works not with all business cases. Better solution would be to save the user decision in the DB. I can add a column to the user table or adding a new table (user_pk, send_mail), which should it be?

update: with this commit the solution with a new column is added

[goekay]

hey @fnkbsi, i want to proceed with this PR and here is my feedback:

• i think two new TransactionRepository methods are not necessary. you are making 2 DB calls (worst case) in your NotificationService impl: first for "is there an active transaction" and second for "give me data of this transaction". this can be done in one call. we can extend getTransactions(TransactionQueryForm form) to support connectorId as query param. then, you would have everything you need and can use one DB call.
• i personally think list of UserNotificationFeatures for the user overview page might be too crowded. it should be enough/fine to see them only on user detail page. WDYT?
• StringUtils.isValidAddress should not be necessary during operation (i.e. check when sending email). if an email is invalid, it should not enter our system and database. validation should occur during input. see @Email validation. why do you think this is needed, additionally?
• i think NotificationService became too crowded handling two concerns: system/admin mails, and user mails. what about creating a UserNotificationService and leaving NotificationService as is? you can put the @EventListener on its methods just fine. then, this event will have two listeners.
• the if-else block nesting is a bit too much NotificationService. i would kindly suggest to us the early exit (or early return) pattern to keep the flow linear.
• String recipientAddresses should not enter a business service (MailService) like this IMO. it should be List<String> recipientAddresses. data transformation etc. should be handled outside of it.

thanks!

[fnkbsi]

hey @fnkbsi, i want to proceed with this PR and here is my feedback:

• i think two new TransactionRepository methods are not necessary. you are making 2 DB calls (worst case) in your NotificationService impl: first for "is there an active transaction" and second for "give me data of this transaction". this can be done in one call. we can extend getTransactions(TransactionQueryForm form) to support connectorId as query param. then, you would have everything you need and can use one DB call. -->Good point, pls check the revised version

• i personally think list of UserNotificationFeatures for the user overview page might be too crowded. it should be enough/fine to see them only on user detail page. WDYT? --> My idea behind this was: the Detail-Site is for changes, the overview also for users with less rights. So the user with less rights can also check if notification should be send. Not sure this is really a use-case. If not it's easy to remove it from the js-file.

• StringUtils.isValidAddress should not be necessary during operation (i.e. check when sending email). if an email is invalid, it should not enter our system and database. validation should occur during input. see @Email validation. why do you think this is needed, additionally? --> Should be true. But I didn't changed it yet.

• i think NotificationService became too crowded handling two concerns: system/admin mails, and user mails. what about creating a UserNotificationService and leaving NotificationService as is? you can put the @EventListener on its methods just fine. then, this event will have two listeners. --> may be a good idea, but I have no time to implement and test it

• the if-else block nesting is a bit too much NotificationService. i would kindly suggest to us the early exit (or early return) pattern to keep the flow linear. --> accepted and changed

• String recipientAddresses should not enter a business service (MailService) like this IMO. it should be List<String> recipientAddresses. data transformation etc. should be handled outside of it. --> accepted and changed

thanks!

[goekay]

whoever is (still) interested: i think the current state of the PR is production-ready and can be merged. pls take a final look. i aim to merge in a couple of days.

thank you @fnkbsi !