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

Fix Citrix segment filter #9251

Merged
merged 1 commit into from Nov 21, 2020
Merged

Conversation

kuzmany
Copy link
Member

@kuzmany kuzmany commented Sep 28, 2020

Q A
Branch? staging for features or enhancements / 3.0 for bug fixes
Bug fix? yes/no
New feature? yes/no
Deprecations? yes/no
BC breaks? yes/no
Automated tests included? yes/no
Related user documentation PR URL mautic/mautic-documentation#...
Related developer documentation PR URL mautic/developer-documentation#...
Issue(s) addressed Fixes #...

Description:

Segment filters for Citrix services are wrong in M3 due array_flip changes. This PR fixed it

Steps to test this PR:

  1. Load up this PR
  2. Setup Citrix plugin and Go To Webinar https://docs.mautic.org/en/plugins/citrix
  3. Sync data php bin/console mautic:citrix:sync
  4. Go to segment and try Webinars segment filter
  5. Without fix you see wrong filters, after fix are correct

Before:

image

After

image

@kuzmany kuzmany added bug Issues or PR's relating to bugs ready-to-test PR's that are ready to test labels Sep 28, 2020
@kuzmany kuzmany added this to the 3.1.2 milestone Sep 28, 2020
@cla-bot cla-bot bot added the cla-signed The PR contributors have signed the contributors agreement label Sep 28, 2020
@npracht npracht added email-integrations Anything related to integrations with mail providers. Plugins have a separate label! segments Anything related to segments labels Oct 9, 2020
@npracht
Copy link
Member

npracht commented Oct 12, 2020

@flossels, @ekkeguembel, @luguenth can you please have a look at this with your experience on GoTo, that would be highly appreciated.
Thanks !

@npracht npracht closed this Oct 12, 2020
@npracht npracht reopened this Oct 12, 2020
@codecov
Copy link

codecov bot commented Oct 12, 2020

Codecov Report

Merging #9251 (c8af96b) into staging (8d5c970) will not change coverage.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             staging    #9251   +/-   ##
==========================================
  Coverage      31.17%   31.17%           
  Complexity     33546    33546           
==========================================
  Files           1943     1943           
  Lines         116002   116002           
==========================================
  Hits           36162    36162           
  Misses         79840    79840           
Impacted Files Coverage Δ Complexity Δ
...auticCitrixBundle/EventListener/LeadSubscriber.php 0.00% <0.00%> (ø) 35.00 <0.00> (ø)

@dennisameling dennisameling changed the base branch from staging to 3.1 October 17, 2020 18:50
@npracht npracht modified the milestones: 3.1.2, 3.2 Oct 19, 2020
luguenth
luguenth previously approved these changes Nov 18, 2020
Copy link
Contributor

@luguenth luguenth left a comment

Choose a reason for hiding this comment

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

Works fine for me 👍

@npracht npracht 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 Nov 18, 2020
@RCheesley RCheesley closed this Nov 20, 2020
@RCheesley RCheesley reopened this Nov 20, 2020
@RCheesley RCheesley changed the base branch from 3.1 to staging November 20, 2020 20:53
@RCheesley RCheesley dismissed luguenth’s stale review November 20, 2020 20:53

The base branch was changed.

@RCheesley RCheesley added the T1 Low difficulty to fix (issue) or test (PR) label Nov 20, 2020
@RCheesley
Copy link
Sponsor Member

I think this is good to go based on @luguenth 's +1 as it's a simple change we have made in many other places since 3.0. Just updated the base branch to staging, will need to check whether a rebase is required.

Copy link
Sponsor Member

@RCheesley RCheesley left a comment

Choose a reason for hiding this comment

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

👍 based on the earlier reviews

@mautibot
Copy link

mautibot commented Dec 1, 2020

This pull request has been mentioned on Mautic Community Forums. There might be relevant details there:

https://forum.mautic.org/t/mautic-3-2-growing-together/17244/1

@dsp76
Copy link

dsp76 commented Oct 11, 2022

Could it be, that this PR somehow is not in version 4.4.3 anymore? We upgraded a client and experiencing exactly the issue mentioned before the PR. However, shouldn't this PR now be merged also in version 4?

@RCheesley
Copy link
Sponsor Member

@dsp76 please raise a new issue if the problem has recurred - you can reference this PR if it previously fixed the issue! Thanks!

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 cla-signed The PR contributors have signed the contributors agreement email-integrations Anything related to integrations with mail providers. Plugins have a separate label! pending-test-confirmation PR's that require one test before they can be merged segments Anything related to segments T1 Low difficulty to fix (issue) or test (PR)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants