Add a recipe that sends notifications about new comments

[jurgenhaas]

Resolves #99

[phenaproxima]

What is this recipe? Where can I see it?

Also, this only brings in the recipe as a dependency. It doesn't actually reference it or apply it anywhere. It's not added to the list of curated recipes available to the project browser.

We probably need to go a little bit further here.

[jurgenhaas]

Yes, how do you want that to happen? What's needed to make it available to the project browser? I'm not sure what the current state of that development is, I'm aware there's a lot of discussions going on with it.

[jurgenhaas]

What is this recipe? Where can I see it?

The recipe can be found at https://gitlab.lakedrops.com/drupal/recipes/notify_new_comments

[phenaproxima]

To make it available to Project Browser, you'd alter recipes/starshot/recipe.yml, specifically this bit:

    project_browser.admin_settings:
      simple_config_update:
        enabled_sources:
          # Don't bother showing modules; just list recipes!
          - recipes
        allowed_projects:
          recipes:
            - starshot_multilingual

And you'd add it by name to the list of allowed projects. (Right after starshot_multilingual).

But I'm not entirely certain we want to have this as a curated recipe. Maybe we should just enable it unconditionally for the starshot_blog recipe. Which reminds me -- is there a way we can configure the notify_new_comments ECA entity to only work for blog posts? Maybe via a config action? How would we go about that?

[jurgenhaas]

Good idea to add this as a recipe to the blog recipe, then we don't have to do anything else, I guess. I've added it as recipes/notify_new_comments, not sure if that path is the correct one, though.

WRT to the configuration to only notify for comments on blogs, it can be done. But that needs some thoughts first. As this only notifies on comments in the first place, is there anything else that contains comments which isn't a blog? Not for now, but I see that could be the case later. But then we need to consider that multiple content types may want to enable that notify recipe and enable it for their content type too. With that in mind, that's still possible but needs a different solution. Let's first define, what we really want in general.

[jurgenhaas]

@phenaproxima I'm not sure why tests are failing now. The recipe get installed, but during installation it fails with this message:

In RecipeConfigurator.php line 63:

  Can not find the notify_new_comments recipe, search path: /var/www/vendor/composer/../..//recipes

It seems that the web root directory is missing?

[phenaproxima]

You know, that's interesting...I wonder why it's not looking in web/recipes, where all the recipes should be? I'll have to dig into that.

[phenaproxima]

You know, I bet this has something to do with the fact that all the component recipes are symlinked into web/recipes by Composer.

That wouldn't happen on a Real Site, but since we don't publish the component recipes to Packagist (and won't, for the foreseeable future), maybe we can find some workaround.

[phenaproxima]

Figured it out -- it's a flaw in the installer. Nicely caught!

[jurgenhaas]

That was quick! Thanks for fixing it.

[phenaproxima]

I agree that we should think this through.

I have a question -- what if I, as a site administrator, want to disable this? Where would I go? I see that ECA's UI is not enabled, but maybe it should be (presumably by the notify_new_comments recipe itself).

Also, what if I wanted to turn off commenting for particular nodes? Is there a way to do that? Could we add a field for that? I think that'd be pretty cool, and would add value to this recipe (and to Starshot itself).

And, a request -- I see that the notify_new_comments recipe has a wide-open constraint for drupal/eca_content. That's a dangerous practice, because it allows Composer to resolve to ANY version of ECA, including ones that might not actually be compatible. Can you change that constraint to drupal/eca:^2, so it's explicit? Additionally, let's add a requirement for drupal/core:^10.3, which is the first version of core that supports recipes.

[jurgenhaas]

I have a question -- what if I, as a site administrator, want to disable this? Where would I go? I see that ECA's UI is not enabled, but maybe it should be (presumably by the notify_new_comments recipe itself).

Good idea. When we enable the eca_ui then individual models can be disabled or even deleted. I'll add that to the recipe.

Also, what if I wanted to turn off commenting for particular nodes? Is there a way to do that? Could we add a field for that? I think that'd be pretty cool, and would add value to this recipe (and to Starshot itself).

Isn't this a Drupal core feature that comes with the comment field? By default, commenting is open, but can be disabled or hidden on a per node base. Or do you want to disable notifications on a per node basis? If the latter, then we would have to store that somewhere, e.g. in a checkbox field which is available in the form but not in the views? Could indeed be added to the recipe.

And, a request -- I see that the notify_new_comments recipe has a wide-open constraint for drupal/eca_content. That's a dangerous practice, because it allows Composer to resolve to ANY version of ECA, including ones that might not actually be compatible. Can you change that constraint to drupal/eca:^2, so it's explicit? Additionally, let's add a requirement for drupal/core:^10.3, which is the first version of core that supports recipes.

As for ECA (or eca_content), the recipe in fact works with all version of ECA, that's why this is open. And as we generate the recipes automatically as an export feature, we can't really determine that constraint. But composer would only always install an ECA version that's compatible with the context, so that doesn't sound too bad. However, if that's a "no go" then we would have to add something to the export framework in ECA.

As for the Drupal core dependency, it would be better to use >=10.3 so that it covers future majors of Drupal as well. However, like before, when downloading a recipe package with composer on Drupal 10.2, that wouldn't do any harm, it just couldn't be applied.

[phenaproxima]

Isn't this a Drupal core feature that comes with the comment field? By default, commenting is open, but can be disabled or hidden on a per node base.

You're right! If Drupal core already provides it, no need for us to do anything else.

[phenaproxima]

As for the Drupal core dependency, it would be better to use >=10.3 so that it covers future majors of Drupal as well.

That's a better idea! Let's do that. You're right that it would be harmless to download recipes on Drupal 10.2, but it would also just be useless cruft.

[jurgenhaas]

When we enable the eca_ui then individual models can be disabled or even deleted. I'll add that to the recipe.

I've added that to the starshot_blog recipe as it is required there to provide that disable option for the site builder, but it's not required for the notify recipe as such, as it could be used in other context too.

[phenaproxima]

Bit of a nitpick, but I decided to move the eca_ui install to the main starshot recipe, since it's technically not required for the blog recipe either, if you like what it does and don't want to change it.

[jurgenhaas]

Great move, this is then also available for other ECA related recipes in the future, that also may want to disable something.

[jurgenhaas]

The notify model now also comes with the drupal/core dependency.

[phenaproxima]

I think all that remains is for me to manually test it (with DDEV, naturally, since it includes a mail catcher, AFAICT?) and then merge!

[jurgenhaas]

Yes a thought: installing eca_ui by the main starshot recipe means, you need to require drupal/eca in the main starshot composer.json as well, don't you?

[phenaproxima]

Yes a thought: installing eca_ui by the main starshot recipe means, you need to require drupal/eca in the main starshot composer.json as well, don't you?

I was on the fence about that, but you're right; it's probably a smart idea to be explicit about that.

[phenaproxima]

Tested the notification feature with DDEV and it worked beautifully.

As a final thought before I merge this: is there any value in having the message include the text of the comment? I feel like that's preferable to showing a link (which doesn't go directly to the comment). What do you think?

[jurgenhaas]

Tested the notification feature with DDEV and it worked beautifully.

Nice!!!

As a final thought before I merge this: is there any value in having the message include the text of the comment? I feel like that's preferable to showing a link (which doesn't go directly to the comment). What do you think?

Sure, have updated the model in the notify recipe. I still left the link to the commented content in there, since that's the way the author can review and probably also reply to that comment then.

[phenaproxima]

Tested again and it worked a treat. I'm into this. Merging. Congrats - it's the first third-party recipe to be added to the prototype!

[jurgenhaas]

Tested again and it worked a treat. I'm into this. Merging. Congrats - it's the first third-party recipe to be added to the prototype!

Oh wow, what an honor 😄 Thank you so much for working on this with me, that was a nice experience.

[gitressa]

That was an extremely impressive display of efficiency by both of you, thank you! And so great to get ECA into Starshot.

[jurgenhaas]

@phenaproxima there have been 2 suggestions about the notify recipe in itself. I've created 2 issues for that in the recipe project at https://gitlab.lakedrops.com/drupal/recipes/notify_new_comments/-/issues and will be implementing them. Hope that's OK with you as well.

Also, a third suggestion asked for also adding bpmn_io as a dependency to the main Starshot recipe next to ECA. It shouldn't be enabled by default, but it should be available if a user wants to go further. Should I open an issue for that?

[phenaproxima]

Those two issues seem like good ideas to me, @jurgenhaas.

As for bpmn_io -- we could add it as a dependency if there's some actual use for it. I don't think we should be adding modules that we're not immediately using to add something tangible and useful. What does that module do? Why would we want it? What is the usefulness?

[jurgenhaas]

The bpmn_io module is the graphical interface to edit existing ECA models or build new ones. In the context of this issue, this would be about modifying the notification behaviour that comes with this recipe.

I would expect the initial audience of Starshot to not use it on day 1, but most likely later on when they're more familiar with Drupal.

But I can see that project browser would be around to let such users download and install bpmn_io themselves.

[phenaproxima]

Yeah, I think I'd rather go that route. :)

[jurgenhaas]

That's OK. I've updated the ECA model to v3 which will be pulled from now on. And the bpmn UI is left to the project browser to make is discoverable there if users are searching for that functionality.

[gitressa]

@jurgenhaas: Perhaps it could be considered to do some restructuring in ECA, and consolidate the module a bit, as I suggested in ECA: Idea for project page to help with on-boarding?

Proposed resolution

Choose a single model (BPMN.iO?) and use that everywhere [...]

Remaining tasks

Decide if the BPMN.iO modeller should be singled out as the default. The two alternatives can of course still be mentioned, but be positioned less prominently.

This would make it easier to get started with using ECA, it's hard enough already, with many moving parts. Also, it would allow Starshot users to immediately tweak the ECA model(s).

What would be the downside of making BPMN.iO an integral part of ECA, and the default modeller?

[jurgenhaas]

Perhaps it could be considered to do some restructuring in ECA, and consolidate the module a bit, as I suggested in ECA: Idea for project page to help with on-boarding?

That's about restructuring the project's main page, not the module itself. At least that's how I understood that. And yes, this is open for contribution, we certainly need to improve our marketing and the user support here.

What would be the downside of making BPMN.iO an integral part of ECA, and the default modeller?

Proper system architecture is a reason against that. ECA is a black box processor with a single dependency: Drupal core. And that is important, short term and long term.

Modellers like bpmn_io are optional, even if there were only one available.

---

However, making the UI discoverable for end users and more intuitive is our top priority now that ECA 2.0 has been released.

[gitressa]

Thanks for a fast answer, and yes you're right. The Drupal issue comment was about making choices for people, and giving the users a 1-2-3 ready! recipe of two commands (composer require + drush install) which downloads and installs everything required to start using ECA.

And true, I went a step further here, and suggested even packaging within ECA the BPMN.iO modeller, as a dependency of ECA. So this is what I propose should be the steps required to install ECA and BPMN.iO, to get started really quickly, and not having to figure out what a modeller even is, and choose the correct one (most everybody will probably use bpmn_io anyway):

composer require drupal/eca
drush in eca eca_base eca_ui bpmn_io

After that, ECA should be fully functional, with BPMN.iO as the modeller.

What would be the downside of making BPMN.iO an integral part of ECA, and the default modeller?

Proper system architecture is a reason against that. ECA is a black box processor with a single dependency: Drupal core. And that is important, short term and long term.

Modellers like bpmn_io are optional, even if there were only one available.

I very much agree.

It's a great idea to be modeller agnostic, and able to switch to any other modeller, other than BPMN.iO, if so desired. That's perfectly fine. However, as I see it, ECA should be opinionated out-of-the-box, and ship with a sane default modeller, to make on-boarding new users easier. With the current complexity, we are shooting ourselves in the foot, and creating a bigger barrier of entry, than it needs to be.

I agree, let's work on improving the UI and documentation. And once again, thanks for working on ECA, it has enormous potential.

[jurgenhaas]

ECA should be opinionated out-of-the-box, and ship with a sane default modeller, to make on-boarding new users easier. With the current complexity, we are shooting ourselves in the foot, and creating a bigger barrier of entry, than it needs to be.

We could turn this around. So far, the issue you're describing is because we put ECA in the centre of the "universe".

Putting ourselves into the shows of a non-Drupalist, shouldn't we push BPMN as a tool for them top be in control of what their Drupal site can do for them. That would then come with the integrated ECA, which for that audience is not that relevant. What matters to them, there is a great UI that let's them build applications without programming.

In other words, we may have to start positioning BPMN as the no-code solution for Drupal in its own right. And ECA just comes with it, but not on the front page.

Sorry, for discussing all this in this PR, where it's completely off-topic. We should have that discussion over in ECA's issue queue or Slack channel.

[gitressa]

That's a really interesting new perspective ... perhaps it could result in a no-code revolution in Drupal?

But yes! Let's move it over to ECA in a new issue. I created Position BPMN as the no-code solution for Drupal and copied over the relevant bits.