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

Applying publishers to receive notifications should not be a config setting #218

Open
elfacht opened this issue Oct 21, 2024 · 10 comments
Open
Labels
feature request New feature or request

Comments

@elfacht
Copy link

elfacht commented Oct 21, 2024

What are you trying to do?

The setting Publishers To Receive Notifications can be changed on production environments, but will cause changes in the config.yml. These changes will be overwritten by the next deployment.

Making changes here:

Screen shot 2024-10-21 um 13 45 52

… will cause this:

Screen shot 2024-10-21 um 14 28 06

Screen shot 2024-10-21 um 13 42 03

This has a huge conflict potential for deployments!

What's your proposed solution?

Settings which are available in production environments should be DB based and not config based. These changes should only take effect on the environments where they were applied. Right now it requires a developer to add/remove publishers, although there is a public setting.

Additional context

No response

@elfacht elfacht added the feature request New feature or request label Oct 21, 2024
@elfacht elfacht changed the title Applying publishers to notification should not be a config setting Applying publishers to receive notifications should not be a config setting Oct 21, 2024
@elfacht
Copy link
Author

elfacht commented Oct 21, 2024

This also causing problems since one user has another ID on different environments, so the settings won't apply on production.

@engram-design
Copy link
Member

Those settings shouldn't be accessible unless allowAdminChanges is set to true? You're not meant to edit them per-environment, as they are plugin settings.

And yes, granted selecting individual user elements isn't exactly ideal as a plugin setting value, so we might change that to a user group instead. That'll be a little bit of effort to get around being a breaking change, but doable.

@elfacht
Copy link
Author

elfacht commented Oct 22, 2024

@engram-design allowAdminChanges is set to trueon our test systems, but not live. This isn't the problem anyway. The problem is, that we have 50+ editors on the live system and their IDs are not in sync with every test system and local dev environments. Therefore setting the users should only be able on the dedicated environments and not by a developer who needs to commit the changes via Git or something.

We are already encounter this exact problem. We can't apply users because we don't have their production IDs in sync locally since user accounts are user generated content and shouldn't be part of config settings.

Users that can create user user accounts (and assign user roles) should be able to assign users to receive publisher notifications. This shouldn't be a developer's job.

@engram-design
Copy link
Member

Right, your comment:

The setting Publishers To Receive Notifications can be changed on production environments

Threw me a little - you shouldn't be able to change it on production environments, and you're saying it can?

But yes, I agree that the notification setting should select a user group that you create during dev, then you can associate users to that group per-environment without messing around with project config changes.

@elfacht
Copy link
Author

elfacht commented Oct 22, 2024

Threw me a little - you shouldn't be able to change it on production environments, and you're saying it can?

That was confusing explained from my site. We haven't deployed it to production, yet. I meant the staging clusters which have the allowAdminChanges enabled in our environment.

@elfacht
Copy link
Author

elfacht commented Nov 14, 2024

Is there any intention to implement this feature anytime soon? Otherwise we would move on to another solution.

@engram-design
Copy link
Member

My concern with this, is it's going to be a breaking change. Anyone currently referencing a User element is going to not work correctly, or have that setting removed, because we can't really translate a User to a User Group.

There's a clunky solution to have two settings for this, one (legacy) for users, the other for user groups, but I'd rather not complicate this setting for that.

My suggestion until then is to download your production database locally, set up the settings, and deploy that. I know that's far from ideal, but I'll look into options.

@elfacht
Copy link
Author

elfacht commented Nov 19, 2024

@engram-design Ok, thanks for your answer and effort. This is unfortunately not feasible with our environment and workflow, so we have to move on. I will have a look at this plugin from time to time, though.

@MoritzLost
Copy link

MoritzLost commented Dec 10, 2024

@engram-design I agree that specific user IDs should never be tracked in the project config. I don't think it's a huge breaking change, as the current solution doesn't really work. It will only work if you create your users in a dev/staging environment and then upload the database to production. This might happen while a project is being developed (though I don't see a good reason to do that). Or shudders it will work if you enable admin changes in production, which shouldn't really be encouraged. Otherwise, there's no way to select user accounts in your dev environment that only exist in a production environment.

But I think there are backwards-compatible ways to address this. You could change the checkboxes field for users to an input field that accepts a comma-separated list of IDs, or an environment variable or Twig template that contains / evalutes to a list of IDs. That's how the built-in condition rules allow you to specify authors without including specific IDs in the project config:

Screenshot 2024-12-10 at 12 13 57

Granted, this only supports a single ID, but there's no reason to use the full condition builder framework for this setting.

Then to support existing configurations, the existing setting could be migrated to a comma-separated list in that field.

@engram-design
Copy link
Member

Added a new setting for this for the next release. To get this early, run composer require verbb/workflow:"dev-craft-4 as 2.0.13".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants