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

[BUGFIX] Add plugin-enabled check condition to CustomObjectPermissions… #292

Open
wants to merge 4 commits into
base: staging
Choose a base branch
from

Conversation

Moongazer
Copy link

To solve the issue #290, tag 1.0.0 (94ff1e5) was checked out. Because this resulted in a detached head, I had to create a branch which is fix/issue-290.

escopecz
escopecz previously approved these changes Dec 7, 2022
Copy link
Contributor

@escopecz escopecz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. Thanks! 👍

@escopecz escopecz requested review from a team, ts-navghane and rohitp19 and removed request for a team and rohitp19 December 7, 2022 09:10
@escopecz
Copy link
Contributor

escopecz commented Dec 7, 2022

I noticed your comment and testing it you are right. It's checking the feature flag which is true by default. Let me see if I'll find a way.

ts-navghane
ts-navghane previously approved these changes Dec 7, 2022
Copy link
Contributor

@ts-navghane ts-navghane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks @Moongazer!

@escopecz
Copy link
Contributor

escopecz commented Dec 7, 2022

We'll have to extend the https://github.com/acquia/mc-cs-plugin-custom-objects/blob/staging/Provider/ConfigProvider.php#L22 to have the Doctrine's Connection service dependency and then check that the plugin is installed properly by checking that the row exists for this plugin in the plugins table at least if not also check if at least some of the custom_object tables exist. Do you think you can add it to this PR?

@Moongazer
Copy link
Author

Moongazer commented Dec 12, 2022

Do you think you can add it to this PR?

Yes, hope I will find some time ASAP

…r tests if the plugin was installed successfully
@Moongazer Moongazer dismissed stale reviews from ts-navghane and escopecz via 7c75a69 December 17, 2022 16:52
@Moongazer
Copy link
Author

Moongazer commented Dec 17, 2022

Change is done and works (tested by deleting the record from plugins table and renaming the plugin-table to custom_object_xxx). Here is the SQL to re-add the deleted record without executing mautic:plugins:reload:

INSERT INTO `plugins` (`name`, `description`, `is_missing`, `bundle`, `version`, `author`) VALUES
('Custom Objects', 'Adds custom objects and fields features to Mautic', 0, 'CustomObjectsBundle', '0.0.19', 'Mautic, Inc.');

Not sure if you like the try-catch block in case of Doctrine exceptions, usually these queries should not fail, so I could also remove it if you want.

@Moongazer Moongazer changed the title [BUGIX] Add plugin-enabled check condition to CustomObjectPermissions… [BUGFIX] Add plugin-enabled check condition to CustomObjectPermissions… Dec 17, 2022
Copy link
Contributor

@escopecz escopecz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please still check for (bool) $this->coreParametersHelper->get(self::CONFIG_PARAM_ENABLED, true);? We heavily depend on that as not all customers want to use this plugin. So we use this feature flag to turn it on only for those who want to use it. From the performance stand point it would be great to first check if the feature flag (the configuration) is ON. If it is off then do not execute the SQL queries at all. If it is on then also check that the plugin is installed with the queries.

@Moongazer
Copy link
Author

OK done.

So we use this feature flag to turn it on only for those who want to use it.

Question: where exactly you turn this flag on/off? I could not find it anywhere, that's why it returned always true for me if the plugin was installed once.

@escopecz
Copy link
Contributor

It is set as true by default so it wouldn't complicate the installation process. Here is the documentation for the feature flag: https://github.com/acquia/mc-cs-plugin-custom-objects/wiki/Feature-flag

escopecz
escopecz previously approved these changes Dec 20, 2022
Copy link
Contributor

@escopecz escopecz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! 👍

Copy link
Contributor

@ts-navghane ts-navghane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thank you @Moongazer

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

Successfully merging this pull request may close these issues.

4 participants