Issue #3305919 Add hook_entity_access() and overwrite the 'view' access

[tbsiqueira]

Problem

When a person is directly added to a event, they do not receive a notification via email.

Solution

There is a message template member_added_by_event_organiser that should be sent to the user, but this never happens. The issue is that in ActivitySendEmailJobType.php we always check if the recipient has access to the entity, before sending out an email. This is not the case, when adding users directly to the event, since the owner of the event enrollment is not the recipient but the event manager, so the condition evaluates to false

We need to allow view access to event enrollment entity if the user is a recipient, since it make sense that the user has access to its own event enrollment.

Issue tracker

https://www.drupal.org/project/social/issues/3305919

Theme issue tracker

[Required if applicable] Paste a link to the drupal.org theme issue queue item, either from socialbase or socialblue. If any other issue trackers were used, include links to those too.

How to test

1. Make sure you have emails sending working
2. Create an event
3. Go to the event and click the Manage enrollments tab
4. Open the Add enrollees menu and click the Add directly option
5. Select a random authenticated user to add
6. Run cron
7. Check if the user received an email that it has been added to the event.

Definition of done

Before merge

• [x] Code/peer review is completed
• [x] All commit messages are clear and clean. If applicable a rebase was performed
• [ ] All automated tests are green
• [ ] Functional/manual tests of the acceptance criteria are approved
• [ ] All acceptance criteria were met
• [ ] New features or changes to existing features are covered by tests, either unit (preferably) or behat
• [ ] Update path is tested. New hook_updates should respect update order, right naming convention and consider hook_post_update code
• [ ] Module can be safely uninstalled. Update/implement hook_uninstall and make sure that removed configuration or dependencies are removed/uninstalled
• [ ] This pull request has all required labels (team/type/priority)
• [ ] This pull request has a milestone
• [ ] This pull request has an assignee (if applicable)
• [ ] Any front end changes are tested on all major browsers
• [ ] New UI elements, or changes on UI elements are approved by the design team
• [ ] New features, or feature changes are approved by the product owner

After merge

• [ ] Code is tested on all branches that it has been cherry-picked
• [ ] Update hook number might need adjustment, make sure they have the correct order
• [ ] The Drupal.org ticket(s) are updated according to this pull request status

Screenshots

[Required if new feature, and if applicable] If this Pull Request makes visual changes then please include some screenshots that show what has changed here. A before and after screenshot helps the reviewer determine what changes were made.

Release notes

Fixed an issue with email sending when adding users directly to the event.

Change Record

[Required if applicable] If this Pull Request changes the way that developers should do things or introduces a new API for developers then a change record to document this is needed. Please provide a draft for a change record or a link to an unpublished change record below. Existing change records can be consulted as example. Please provide a draft for a change record or a link to an unpublished change record below. Existing change records can be consulted as example.

Translations

[Optional]Translatable strings are always extracted from the latest development branch. To ensure translations remain available for platforms running older versions of Open Social the original string should be added to translations.php when it's changed or removed.

• [ ] Changed or removed source strings are added to the translations.php file.

[mergeable[bot]]

Thanks for contributing towards Open Social! A maintainer from the @goalgorilla/maintainers group might not review all changes from all teams/contributors. Please don't be discouraged if it takes a while. In the meantime, we have some automated checks running and it might be that you will see our comments with some tips or requests to speed up the review process. :blush:

[tbsiqueira]

This PR recreates the following: https://github.com/goalgorilla/open_social/pull/3078

[open-social-tugboat]

Tugboat has finished building the preview for this pull request!

Link:

• https://pr3080-j1aaxzsrthyhbkjm3oafnpe76xnlf4bs.tugboatqa.com

Dashboard:

• https://dashboard.tugboatqa.com/63078034a982dc5767b9b2ae

[tbsiqueira]

Hi @nkoporec thank you for your work.

Is this covered by a behat test? If so please make sure to cover this change on a test. It will be interesting to comment on the expected change after this change, this will be useful for the community.

[nkoporec]

@tbsiqueira This is ready for another review.

[Kingdutch]

👋 Spotted this while looking at the state of our CI queue. I think the issue's title is a lot more clear about the "Why" (since our code already documents "What") so I've mirrored the issue title to the PR :D

Loving the addition of a unit test for our access check here <3 Is there any reason we're moving the new function into a Helper rather than keeping it in the access hook directly?

Not having that indirection gives us a few benefits:

• It's easier to see what's happening. The function contains comments of what's happening but the hook implementation doesn't give any information on why it exists.
• We ensure the logic isn't re-used other than through Drupal's access system so we don't accidentally duplicate all the checks in another hook or bypass other hooks by calling the helper function directly.
• Not adding to the helper makes it easier to eliminate these "catch-all" classes in the future.

We should be able to still unit test when all the code is in the hook! 🤩 I'm not sure if we can call the function directly (e.g. our autoloader already loads .module files) or we can include the .module file that we're testing within our test function :)

[robertragas]

👋 Spotted this while looking at the state of our CI queue. I think the issue's title is a lot more clear about the "Why" (since our code already documents "What") so I've mirrored the issue title to the PR :D

Loving the addition of a unit test for our access check here <3 Is there any reason we're moving the new function into a Helper rather than keeping it in the access hook directly?

Not having that indirection gives us a few benefits:

• It's easier to see what's happening. The function contains comments of what's happening but the hook implementation doesn't give any information on why it exists.
• We ensure the logic isn't re-used other than through Drupal's access system so we don't accidentally duplicate all the checks in another hook or bypass other hooks by calling the helper function directly.
• Not adding to the helper makes it easier to eliminate these "catch-all" classes in the future.

We should be able to still unit test when all the code is in the hook! 🤩 I'm not sure if we can call the function directly (e.g. our autoloader already loads .module files) or we can include the .module file that we're testing within our test function :)

Good one, will see if we can relocate the code.

[robertragas]

So I ran into an issue with rewriting the unit test as we need to include the .module file for the hook_entity_access. I tried a few different methods like the moduleHandler and setting containers but didn't get it to work.

This morning I spoke with @Kingdutch and he was talking to just include it like you can see in the code with require(). To move a bit away from Drupal and more towards PHP. Also as we just need to test this specific function. Maybe he can explain it in more detail.

cc @navneet0693 @ronaldtebrake if you have a certain opinion, feel free to state them.