Cloning issue with Event series and Event instances. #330
Conversation
|
retest |
|
I tested here and this looks good. Reproduction steps (from the linked issue):
|
There was a problem hiding this comment.
Hi, @bharath-kondeti .
Thank you for your PR!
Let's improve it a little bit and avoid hard-coding.
I've proposed a better way to get events bundles from config.
You can read it from there instead of hard-coding in it in the array.
- can you add some comments about your changes in code?
Devs that will work with this code in the future will better understand why this check was added.
openy_gated_content.module
Outdated
| @@ -193,6 +193,10 @@ function openy_gated_content_form_alter(&$form, FormStateInterface $form_state, | |||
| 'eventinstance_live_stream_edit_form', | |||
| 'eventinstance_virtual_meeting_edit_form', | |||
| ]; | |||
| $events_bundle = [ | |||
There was a problem hiding this comment.
It might be replaced to:
$events_bundle = $gcConfig['permissions_entities']['eventseries']
to get this array and to avoid hard coding
There was a problem hiding this comment.
@anpolimus while your suggestion does look like an improvement over the original code change - why do we even need this $events_bundle here? If you check later in the code we do already iterate over the "permissions_entities" config array, taking into account both entity type and bundle. And then the new changes suggest that we check if the bundle (the one we get from the iteration cycle) is included into this new $events_bundle variable taken from the same configuration array we iterate on... it doesn't make any sense to me.
There was a problem hiding this comment.
@duozersk, Actually, we do not have clone functionality on blogs or videos, whereas it exists on eventseries/eventinstances(virtual_meetings, live_streams) and this behavior is being added from reccurring_events module.
Hence, I had to do an additional check to add _clone_form into $allowedForms array only for eventseries/eventinstances not to blogs/videos.
There was a problem hiding this comment.
@bharath-kondeti - OK, I see your motivation. Still you are checking only for the bundle name, and not for the entity type. And the code change gets the bundle names for the "eventseries" entity type (from config), but then affects the "eventinstance" entity types bundles too. It creates "hidden logic"... and the codebase that is hard to maintain.
We do have the "permissions_entities" config - that is the list of entity types/bundles that should have Virtual Y custom permissions handling. The fact that we do have clone functionality only on eventseries/eventinstance entity types doesn't change the thing that we should have custom permissions handling on the node entity type with gc_video and vy_blog_post bundles. And we shouldn't add more meanings to this config or add code that uses just parts of this config...
My motivation here is that we need to write clean, clear and maintainable code. And either use the things we already created in the way they were initially created for (permissions_entities config) or create another thing having another meaning (like permissions_entities_clone config, for instance) if the already existing one doesn't fit our usage. I don't want to see the hidden logic in the code, it won't help us in the long run.
Still I do think that we can use this permissions_entities config as it is without any filtering (based on the fact that currently we don't have clone functionality for blogs and videos). Cause we might add this cloning functionality later on; or another Y might enable it. And having clone form IDs for these 2 bundles doesn't hurt or brake anything - and makes the code much cleaner.
There was a problem hiding this comment.
Another thought (if we are going down the filtering path) - according to the explained intent you want to address the eventinstance/eventseries entity types - then why to filter by bundle names and not by entity types?...
There was a problem hiding this comment.
@duozersk, I have updated the code. Please do verify
@anpolimus, Can we please have testing done on this again.
There was a problem hiding this comment.
@bharath-kondeti Thank you! It looks awesome now!
|
@bharath-kondeti @froboy Should I take steps for review from here? ymcatwincities#135 |
|
Build 3000 |
|
Build 3000 |
|
@AnastasiiaPys , you can start testing once build is ok |
|
Tested. Works as expected. |
Related Issue/Ticket:
ymcatwincities#135
Steps to test:
Quality checks:
Please check these boxes to confirm this PR covers the following cases: