pimcore icon indicating copy to clipboard operation
pimcore copied to clipboard
verified publisher icon pimcore

Added a new option in workflow notificationSettings to ensure that notifications are also sent to inactive users.

Open rahul-pim-dev opened this issue 1 year ago • 5 comments

Added a new option in workflow notificationSettings to ensure that notifications are also sent to inactive users.

see : https://github.com/pimcore/pimcore/issues/16867

rahul-pim-dev avatar Apr 05 '24 13:04 rahul-pim-dev

Review Checklist

  • [ ] Target branch (11.2 for bug fixes, others 11.x)
  • [ ] Tests (if it's testable code, there should be a test for it - get help)
  • [ ] Docs (every functionality needs to be documented, see here)
  • [ ] Migration incl. install.sql (e.g. if the database schema changes, ...)
  • [ ] Upgrade notes (deprecations, important information, migration hints, ...)
  • [ ] Label
  • [ ] Milestone

github-actions[bot] avatar Apr 05 '24 13:04 github-actions[bot]

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

sonarqubecloud[bot] avatar Apr 05 '24 13:04 sonarqubecloud[bot]

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Apr 12 '24 08:04 CLAassistant

@rahul-pim-dev Thanks a lot, could you please sign the CLA?

I would call the current behaviour more like a bug. The workflow engine should not send notifications to inactive users. I would say let's not make this configurable. I would call this more like a "security leak". When someone deactivates a user this user should not get any notifications anymore (and therefore receive potentially internal information). If someone wants to send notifications to users without real access to the Pimcore backend it would be possible to add a user without rights but activate it.

markus-moser avatar Apr 19 '24 09:04 markus-moser

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

sonarqubecloud[bot] avatar Jun 04 '24 11:06 sonarqubecloud[bot]

@rahul-pim-dev Sorry for the late feedback - missed your commit. It LGTM but could you please sign the CLA?

markus-moser avatar Jul 26 '24 07:07 markus-moser

@markus-moser I've signed the CLA. Thanks for letting me know!

rahul-pim-dev avatar Aug 08 '24 06:08 rahul-pim-dev

Quality Gate Passed Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

sonarqubecloud[bot] avatar Aug 16 '24 08:08 sonarqubecloud[bot]

@rahul-pim-dev

I tested it and found a few problems:

  • The email field could be either null or (potentially) an empty string. The empty email check did not work for an empty string.
  • Pimcore notifications should be sent to (active) users without email.

Therefore I improved your PR a bit -> now it's fine and I'll merge it. Thanks a lot!

markus-moser avatar Aug 16 '24 08:08 markus-moser