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

Add new notification features #847

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

juherr
Copy link
Contributor

@juherr juherr commented Jun 23, 2022

Add new notification feature about ChargePoint, User and OccpTag create/update/delete.

The pull request is not yet finished because I'd like to add some other notifications (see TODO).

I have some doubt about the naming too.

@goekay
Copy link
Member

goekay commented Jun 23, 2022

thanks for this! i know it's a draft, but here is the one thing that occurred to me after glancing over it:

there are too many dumb data holder classes who share similar fields and their only reason for existence is for the listener to catch them by the class name. can we unify them? something like:

@Data
public class ChargePointOperation {
  private final int chargeBoxPk;
  private final String chargeBoxId;
  private final NotificationFeature type;
}

and then the listener can be:

@EventListener(condition = "#notification.type == NotificationFeature.ChargePointDeleted")
public void chargePointDeleted(ChargePointOperation notification) {
   // do some cool logic
}

or, if we want to reduce spring magic:

@EventListener
public void chargePointDeleted(ChargePointOperation notification) {
    if (notification.type == NotificationFeature.ChargePointDeleted) {
        // do some cool logic
    }
}

@juherr
Copy link
Contributor Author

juherr commented Jun 24, 2022

You're right about the type of notifications.

All notifications on the same item share the same description because I don't know yet what is helpful to set.
I don't like adding NotificationFeature type in the class because it will allow having a tag feature on a charging station notification.

But I will try the following:

  • a generic notification with NotificationFeature type + int pk (the type determines the reference of the pk)
  • custom notifications when needed

It's a draft because not ready for merge but open to remarks :)
Do you have some others?

Fyi, my next PR will be to add a WebhookNotificationService for a Slack (or other) integration

@juherr
Copy link
Contributor Author

juherr commented Jun 24, 2022

About naming, what is the difference between OcppStation and ChargePoint?

@juherr
Copy link
Contributor Author

juherr commented Jul 6, 2022

Ping @goekay
Please, advice

@goekay
Copy link
Member

goekay commented Jul 7, 2022

sorry, it fell under the radar.

About naming, what is the difference between OcppStation and ChargePoint?

nothing. i am, or was, not consistent with the notions :) both are used interchangeably.

@juherr
Copy link
Contributor Author

juherr commented Sep 5, 2022

update: not dead but less free time on my side ;)

@anirudh-ramesh
Copy link

Hi @juherr, I've experimented with adding webhooks that were called from NotificationService. It's a hack since the WebhookService was called from methods of MailService instead of NotificationService. I also got a chance to add some columns to the MySQL database for storing a single webhook link along with enable/disable flags. I need to clean it up and refactor it in a manner suitable for adding to SteVe's master. But, similarly, not dead but less free time :/

@juherr juherr mentioned this pull request Oct 30, 2022
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