Missing event subject

[ncounter]

Every subscribable Event has its own subject (for RSS and Email) implementation.

[hennevogel]

You should check for which events we send emails/rss...

[ncounter]

You should check for which events we send emails/rss...

I thought about that but still, I wouldn't mind to have a fallback already implemented.
It's part of the interface they should implement by default to me.
Does that harm?

[hennevogel]

I thought about that but still, I wouldn't mind to have a fallback already implemented.
It's part of the interface they should implement by default to me.

What interface? There is no interface to those that requires a subject. Or you mean the interface that might or might not exist in some future?

Does that harm?

Sure. It's unused code without purpose. It will confuse people.

[ncounter]

I thought about that but still, I wouldn't mind to have a fallback already implemented.
It's part of the interface they should implement by default to me.

What interface? There is no interface to those that requires a subject. Or you mean the interface that might or might not exist in some future?

Does that harm?

Sure. It's unused code without purpose. It will confuse people.

The whole module Event (e.g.: Event > Base > Build > BuildSuccess) behaves like a chain of interfaces or so, they just don't have implementation requirements.

Anyway, as long as it is not enforced, let's stick to the ones that are currently used only.

[saraycp]

I thought about that but still, I wouldn't mind to have a fallback already implemented.

In such case, I'd propose implementing the subject method in the parent class without a message but with: raise AbstractMethodCalled.

As we already do in other STI parent classes like ScmPayload.

[ncounter]

I thought about that but still, I wouldn't mind to have a fallback already implemented.

In such case, I'd propose implementing the subject method in the parent class without a message but with: raise AbstractMethodCalled.

As we already do in other STI parent classes like ScmPayload.

That works for me 👍

@hennevogel , @saraycp
After looking at how we send emails, in my understanding we do not filter events by any type in order to decide to send it or not. For instance, we do for web and RSS (see here), but there is nothing like that when sending email. So potentially all Events can become an email, even if they are just meant to be STI parent classes of more specific event types.
I initially implemented a fallback subject text for STI parents, but raising the missing implementation sounds even better to me: it doesn't require maintenance and it would warn us in case we misuse Event classes.

I adapted the code accordingly. Feel free to take a look again. Thank you

[hennevogel]

So potentially all Events can become an email...

Nope, only the ones we allow to create subscriptions for...

[saraycp]

@hennevogel what do you mean by "we don't allow subscriptions...". Is there any control/limitation in the code?
We don't create events for those classes but for their "children" classes, it seems it's a developer decision, I can't find the code that prevent us to do it.

I found it, sorry for the noise:

open-build-service/src/api/app/models/event/base.rb

Line 41 in fb97ad0

def notification_events

[ncounter]

@hennevogel I addressed all your comments by dropping the undesired subject implementation. (cc @saraycp @eduardoj )
Please take a final round of review, thank you

[hennevogel]

I'd love to see by whom here but that needs some refactoring first...