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

Adding Citrix plugin deprecation for M4 so it could be removed in M5 #12367

Merged
merged 7 commits into from
May 25, 2023

Conversation

mabumusa1
Copy link
Member

@mabumusa1 mabumusa1 commented May 19, 2023

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

Description:

Companion PR to #12333

Steps to test this PR:

  1. Open this PR on Gitpod or pull down for testing locally (see docs on testing PRs here)
  2. Try to configure CitrixPlugin, you should see a deprecation notice
  3. Try to run SyncCommand, you should see a deprecation notice

@mabumusa1 mabumusa1 self-assigned this May 19, 2023
@mabumusa1 mabumusa1 added the refactoring The change does not change behavior but improves the code label May 19, 2023
@mabumusa1 mabumusa1 modified the milestone: 4.4.9 May 19, 2023
@codecov
Copy link

codecov bot commented May 19, 2023

Codecov Report

Merging #12367 (5935179) into 4.4 (e77b0fd) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##                4.4   #12367      +/-   ##
============================================
- Coverage     50.29%   50.29%   -0.01%     
- Complexity    35446    35448       +2     
============================================
  Files          2145     2145              
  Lines        106425   106431       +6     
============================================
  Hits          53528    53528              
- Misses        52897    52903       +6     
Impacted Files Coverage Δ
plugins/MauticCitrixBundle/Command/SyncCommand.php 12.00% <0.00%> (-0.50%) ⬇️
...ixBundle/Integration/CitrixAbstractIntegration.php 13.33% <0.00%> (-1.31%) ⬇️

@mabumusa1 mabumusa1 added the code-review-needed PR's that require a code review before merging label May 19, 2023
@mabumusa1 mabumusa1 requested a review from escopecz May 19, 2023 16:45
@mabumusa1 mabumusa1 mentioned this pull request May 19, 2023
@escopecz
Copy link
Sponsor Member

We did not understand each other. Apologies. I meant that you'd deprecate the Citrix plugin for Mautic 4.4.9 as Ruth suggested. We have to firstly deprecate the plugin in one version and then we can delete it in the next major version. We cannot remove it in 4.4.9 as it is a BC break.

This is the deprecation PR I did for Pipedrive for inspiration: #12364

@mabumusa1 mabumusa1 added deprecation Includes deprecations ready-to-test PR's that are ready to test and removed refactoring The change does not change behavior but improves the code labels May 21, 2023
@mabumusa1 mabumusa1 added this to the 4.4.9 milestone May 21, 2023
@mabumusa1
Copy link
Member Author

Sample outputs
image
image

@mabumusa1
Copy link
Member Author

We did not understand each other. Apologies. I meant that you'd deprecate the Citrix plugin for Mautic 4.4.9 as Ruth suggested. We have to firstly deprecate the plugin in one version and then we can delete it in the next major version. We cannot remove it in 4.4.9 as it is a BC break.

This is the deprecation PR I did for Pipedrive for inspiration: #12364

No worries and thanks for pointing me to the right direction, I updated the PR as required

@escopecz escopecz changed the title Backport: remove citrix plugin Adding Citrix plugin deprecation for M4 so it could be removed in M5 May 22, 2023
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.

@mabumusa1 please create also similar migration to https://github.com/mautic/mautic/pull/12364/files#diff-ac792d02497f71aedb711f6dd2540b2870677153a10b9bf74cd5a9cc196bffaaR12 that will add the deprecation messages to all users if the Citrix plugin is enabled in that Mautic instance.

@RCheesley @mollux Do we need an announcement blog post like we had for Pipedrive (#12364) or is it OK with deprecating it and suggesting an alternative plugin?

@RCheesley
Copy link
Sponsor Member

@RCheesley @mollux Do we need an announcement blog post like we had for Pipedrive (#12364) or is it OK with deprecating it and suggesting an alternative plugin?

I think that if there is an alternative that is open source and freely available, it's helpful to raise more awareness of the process in a blog post which will reach a different audience. If that is the case maybe the maintainers of the plugin would be up for writing it?

@mabumusa1
Copy link
Member Author

@escopecz I added the migration in this commit 095a3d2

@escopecz escopecz self-requested a review May 22, 2023 13:11
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.

Once they forgotten plugin name is corrected this is good to go from code perspective. The command alert works:

bin/console mautic:citrix:sync

                                                                                                                        
 [WARNING] The Citrix plugin is deprecated and will be removed in Mautic 5. See <a                                      
           href="https://www.leuchtfeuer.com/en/mautic/know-how/mautic/gotowebinar-plugin-new-features/"                
           target=”_blank”>an alternative</a> for this plugin.

The notification is created.

And the warning appears in all 3 Citrix plugins:
Screenshot 2023-05-22 at 15 44 56

Co-authored-by: John Linhart <jan@linhart.email>
escopecz
escopecz previously approved these changes May 22, 2023
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.

Good to go from code perspective. Thanks! 👍

@RCheesley
Copy link
Sponsor Member

Before this is merged can we just double check with Ekke and team that it's the right place to direct people (eg current URL or GitHub etc) before we push it out to everyone?

Copy link
Sponsor

@ekkeguembel ekkeguembel left a comment

Choose a reason for hiding this comment

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

Please change deprecation / replacement URL to https://www.leuchtfeuer.com/en/mautic/downloads/mautic-goto-plugin/
(Looks of that page preliminary, content is largely rendered from README.md which will be overhauled prior to 4.4.9... but URL is final : )

@mabumusa1
Copy link
Member Author

Please change deprecation / replacement URL to https://www.leuchtfeuer.com/en/mautic/downloads/mautic-goto-plugin/ (Looks of that page preliminary, content is largely rendered from README.md which will be overhauled prior to 4.4.9... but URL is final : )

Fixed in commit 5334fc3

@mabumusa1 mabumusa1 added ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged and removed ready-to-test PR's that are ready to test labels May 24, 2023
@escopecz escopecz added bug Issues or PR's relating to bugs and removed code-review-needed PR's that require a code review before merging labels May 25, 2023
@escopecz escopecz merged commit 927ece9 into mautic:4.4 May 25, 2023
15 of 18 checks passed
@mabumusa1 mabumusa1 deleted the remove_citrix branch June 1, 2023 20:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issues or PR's relating to bugs deprecation Includes deprecations ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

4 participants