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

remove all pipedrive related files and functions #12081

Merged
merged 8 commits into from May 19, 2023

Conversation

npracht
Copy link
Member

@npracht npracht commented Mar 11, 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? N/A
Related user documentation PR URL Need to be removed from doc
Related developer documentation PR URL
Issue(s) addressed

Description:

Current Pipedrive plugin stopped working due to Pipedrive API deprecations.
Knowing that Mautic Community policy is to not carry in core third party plugins and move them to marketplace, I supply suggest we remove it from core:

  1. No more maintenance
  2. No more issue on mautic/mautic repo for outdated plugin

To do left

@kuzmany we need to remove dead code of Pipedrive on this 2 last files, but I was scared to touch these functions 👀

  • plugins/MauticCrmBundle/EventListener/CompanySubscriber.php
  • plugins/MauticCrmBundle/EventListener/LeadSubscriber.php

@cla-bot cla-bot bot added the cla-signed The PR contributors have signed the contributors agreement label Mar 11, 2023
@npracht npracht added ready-to-test PR's that are ready to test bc-break A BC break PR for major release milestones only labels Mar 11, 2023
@npracht npracht requested a review from kuzmany March 11, 2023 08:35
@npracht npracht force-pushed the fix/remove-pipedrive-plugin branch 2 times, most recently from 8382d9d to 73a93cc Compare March 11, 2023 08:59
@npracht npracht added the plugin Anything related to plugins label Mar 11, 2023
@codecov
Copy link

codecov bot commented Mar 11, 2023

Codecov Report

Merging #12081 (4370fc7) into 5.x (f34eb9f) will decrease coverage by 0.41%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##                5.x   #12081      +/-   ##
============================================
- Coverage     55.89%   55.48%   -0.41%     
+ Complexity    35958    35683     -275     
============================================
  Files          2258     2243      -15     
  Lines        107806   107014     -792     
============================================
- Hits          60255    59381     -874     
- Misses        47551    47633      +82     
Impacted Files Coverage Δ
...ndles/LeadBundle/Entity/CustomFieldEntityTrait.php 91.66% <ø> (-5.21%) ⬇️

... and 26 files with indirect coverage changes

@kuzmany
Copy link
Member

kuzmany commented Mar 13, 2023

I cleanup phpbaseline.neon
Also I've added migration to remove all integration_entity table records (If do not need it I can revert it).

@escopecz escopecz added this to the 5.0-alpha milestone Mar 13, 2023
escopecz
escopecz previously approved these changes Mar 13, 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.

The code changes look good to me. Thanks for removing a plugin that is no longer working. Let's use the new one instead.

@escopecz escopecz added pending-test-confirmation PR's that require one test before they can be merged and removed ready-to-test PR's that are ready to test labels Mar 13, 2023
@escopecz
Copy link
Sponsor Member

@npracht @kuzmany I can see we did not get the second approval for this and now we have some conflicts. Could you please resolve those?

@RCheesley as there is basically nothing to test as it's removing a plugin, can you give your approval on this?

@escopecz escopecz added the has-conflicts Pull requests that cannot be merged until conflicts have been resolved label Apr 11, 2023
@RCheesley
Copy link
Sponsor Member

Happy to approve once the conflicts are resolved and I am able to do some basic testing. Knowing how there have been in the past some issues with removing plugins breaking things I do think we need to do some basic user testing.

I still think that we need to post a front-end notice for people who are using this version of the Pipedrive plugin in a 4.4.x release to notify them that it is going to be removed in Mautic 5.0 and telling them to update to the new plugin, pointing them at the blog post on mautic.org. This is in line with our policy on deprecating features - is that feasible to do in a separate PR please?

I know that folks should have already noticed that the plugin stopped working but we should follow our own guidance as best we can.

@escopecz
Copy link
Sponsor Member

@RCheesley I can create the PR for the UI notices that this plugin will be removed in Mautic 5.0.0 for the Mautic 4.4.9 release. Where should I add the UI notices? In the plugin config form?

@RCheesley
Copy link
Sponsor Member

Where should I add the UI notices? In the plugin config form?

I was thinking that a cancel-able message on the dashboard would be helpful, perhaps? Otherwise we are expecting the user to go to the plugin screen which, let's face it, they usually only do when configuring the plugin. I'd suggest displaying it with the yellow background, with the option to close it via a cross button

If it's possible to only show it when the plugin in question is enabled that would be ideal - this is code that I think we will re-use so maybe having some documented best practice which we can add to the deprecation policy would be helpful in the future?

@escopecz
Copy link
Sponsor Member

@RCheesley we have this notification area in Mautic:
Screenshot 2023-05-18 at 14 56 04
We can easily create a migration to add a message there. Creating some special new notification that we'll need to show only once per each user and remember when they dismiss it would be quite a big feature. As we struggle with resources to finish M5 there is no space to implement something new or make the existing notification center somehow better. Is adding a message there good enough?

@RCheesley
Copy link
Sponsor Member

Is adding a message there good enough?

Yes I think that is fine for what we need!

@escopecz escopecz mentioned this pull request May 19, 2023
@escopecz
Copy link
Sponsor Member

Here is the PR for M4.4 with deprecation messages regarding removal of the PipeDrive plugin. Please review and test so we can get this merged.

#12364

@escopecz escopecz self-requested a review May 19, 2023 15:05
@escopecz escopecz force-pushed the fix/remove-pipedrive-plugin branch from 836bf75 to f227ea4 Compare May 19, 2023 15:10
@escopecz escopecz removed the has-conflicts Pull requests that cannot be merged until conflicts have been resolved label May 19, 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.

I rebased this, fixed a couple of issues. Good to go now 👍

Copy link
Member

@mabumusa1 mabumusa1 left a comment

Choose a reason for hiding this comment

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

Changes looks good, Migration changes are great

@escopecz escopecz merged commit f2fee7a into mautic:5.x May 19, 2023
9 of 11 checks passed
@escopecz escopecz removed the pending-test-confirmation PR's that require one test before they can be merged label May 19, 2023
@escopecz escopecz added the bug Issues or PR's relating to bugs label Jun 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bc-break A BC break PR for major release milestones only bug Issues or PR's relating to bugs cla-signed The PR contributors have signed the contributors agreement plugin Anything related to plugins
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants