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

Add constrain to check if plugin can be published. #13696

Open
wants to merge 30 commits into
base: 5.x
Choose a base branch
from

Conversation

rohitpavaskar
Copy link
Contributor

@rohitpavaskar rohitpavaskar commented Apr 26, 2024

Q A
Bug fix? (use the a.b branch) [N ]
New feature/enhancement? (use the a.x branch) [Y]
Deprecations? [N]
BC breaks? (use the c.x branch) [N]
Automated tests included? [Y]
Related user documentation PR URL mautic/mautic-documentation#...
Related developer documentation PR URL mautic/developer-documentation#...
Issue(s) addressed Fixes #...

Description:

When authorizing the plugin, if my user doesn't have particular access, I should receive an error and fail to authorize the plugin. This i new constrain which we need to add in any plugin which will not allow user to publish the plugin if user don't have sufficient access.
Also clear the access and refresh token whenever credentials are changed.

Example usage in a plugin:

class CanPublishValidatorSubscriber implements EventSubscriberInterface
{
    public static function getSubscribedEvents()
    {
        return [
            PluginEvents::PLUGIN_IS_PUBLISHED_STATE_CHANGING => ['onPublishStateChange', 0],
        ];
    }

    public function onPublishStateChange(PluginIsPublishedEvent $event)
    {
        if ('YourIntegrationName' !== $event->getIntegrationName() || 1 !== $event->getValue()) {
            return;
        }

        // custom logic here...

        $event->setCanPublish($enabled);
        $event->setMessage($message);
    }
}

Steps to test this PR:

  1. Open this PR on Gitpod or pull down for testing locally (see docs on testing PRs here)
  2. Not testable without adding constrain in plugin.

rohitp19 and others added 23 commits April 26, 2024 11:54
…need to do some verification before publishing plugin
…the form validation fails so that it will not publish plugin with errors. If already in publish state do not unpublish as it may cause other things to fail
Co-authored-by: Miroslav Fedeleš <[email protected]>
Copy link

codecov bot commented Apr 30, 2024

Codecov Report

Attention: Patch coverage is 85.93750% with 9 lines in your changes are missing coverage. Please review.

Project coverage is 61.64%. Comparing base (4883fa0) to head (74fabfc).

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##                5.x   #13696      +/-   ##
============================================
+ Coverage     61.62%   61.64%   +0.01%     
- Complexity    34145    34169      +24     
============================================
  Files          2245     2249       +4     
  Lines        102077   102134      +57     
============================================
+ Hits          62909    62963      +54     
- Misses        39168    39171       +3     
Files Coverage Δ
...dles/PluginBundle/Event/PluginIsPublishedEvent.php 100.00% <100.00%> (ø)
...ndles/PluginBundle/Exception/ApiErrorException.php 41.17% <100.00%> (+24.50%) ⬆️
...undles/PluginBundle/Form/Constraint/CanPublish.php 100.00% <100.00%> (ø)
...uginBundle/Form/Constraint/CanPublishValidator.php 100.00% <100.00%> (ø)
app/bundles/PluginBundle/Form/Type/DetailsType.php 93.33% <100.00%> (+0.11%) ⬆️
...les/PluginBundle/Form/Type/FeatureSettingsType.php 74.07% <100.00%> (ø)
app/bundles/PluginBundle/MauticPluginBundle.php 100.00% <100.00%> (ø)
...ndles/PluginBundle/Controller/PluginController.php 46.31% <83.33%> (+3.21%) ⬆️
...s/PluginBundle/Integration/AbstractIntegration.php 34.00% <33.33%> (+0.33%) ⬆️

... and 1 file with indirect coverage changes

@escopecz escopecz added ready-to-test PR's that are ready to test enhancement Any improvement to an existing feature or functionality plugin Anything related to plugins unforking Used for PRs in the Acquia's unforking initiative labels Apr 30, 2024
Copy link
Sponsor Member

@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 Rohit! I found a few places that could improve. Please go through the suggestions and apply them if they make sense.

@escopecz escopecz added the pending-feedback PR's and issues that are awaiting feedback from the author label May 14, 2024
@escopecz escopecz removed the pending-feedback PR's and issues that are awaiting feedback from the author label May 15, 2024
Copy link
Sponsor Member

@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.

Thank you, Rohit! I see no problems in the code changes 👍

@escopecz escopecz added pending-test-confirmation PR's that require one test before they can be merged code-review-passed PRs which have passed code review and removed ready-to-test PR's that are ready to test labels May 15, 2024
@escopecz
Copy link
Sponsor Member

Since this cannot be directly tested without a proprietary plugin, can someone do a second code review? Perhaps @kuzmany ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code-review-passed PRs which have passed code review enhancement Any improvement to an existing feature or functionality pending-test-confirmation PR's that require one test before they can be merged plugin Anything related to plugins unforking Used for PRs in the Acquia's unforking initiative
Projects
Status: ⏳︎ Needs 1 more test
Development

Successfully merging this pull request may close these issues.

None yet

4 participants