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

Asset download segment filter #6182

Merged
merged 6 commits into from Apr 13, 2019
Merged

Conversation

@maxlawton
Copy link
Contributor

@maxlawton maxlawton commented Jun 7, 2018

Q A
Bug fix? N
New feature? Y
Related user documentation PR URL
Related developer documentation PR URL
Issues addressed (#s or URLs)
BC breaks? N
Deprecations? N

Description:

Adds asset multi selector support.
Adds segment filter for leads having downloaded an asset.

Steps to test this PR:

  1. Create an asset.
  2. Download the asset as a lead.
  3. Create segment selecting the created asset.
  4. Update the segment and check if it contains the lead.
@luizeof
Copy link
Contributor

@luizeof luizeof commented Nov 18, 2018

@npracht this will be added to 2.15.0 or 2.15.1?

@npracht
Copy link
Member

@npracht npracht commented Nov 19, 2018

Hi @luizeof ! Until now 2.15.0 is closed, so we can candidate it for 2.15.1 !

@npracht npracht added this to the 2.15.1 milestone Nov 19, 2018
@heathdutton heathdutton added this to Code Review (2 required) in Mautic 2 Dec 6, 2018
@npracht npracht moved this from Code Review (2 required) to Ready to Test (first time) in Mautic 2 Jan 3, 2019
@npracht npracht moved this from Ready to Test (first time) to Code Review (2 required) in Mautic 2 Jan 3, 2019
@alanhartless alanhartless added this to Has Conflicts and/or Failing Tests in 2.15.1 Jan 14, 2019
@npracht
Copy link
Member

@npracht npracht commented Feb 4, 2019

Hi @maxlawton could you please solve conflicts ?
Thanks !

@alanhartless alanhartless moved this from Has Conflicts and/or Failing Tests to Needs Testing in 2.15.1 Mar 5, 2019
@johbuch
Copy link
Contributor

@johbuch johbuch commented Mar 8, 2019

just tested on mautibox.
It does not seems to work
I have downloaded the asset
image
created a segment this filter
image
after the crons runned, no contact in the segment
image

@alanhartless alanhartless removed this from the 2.15.1 milestone Mar 11, 2019
@alanhartless alanhartless added this to the 2.16.0 milestone Mar 11, 2019
@alanhartless alanhartless removed this from Needs Testing in 2.15.1 Mar 11, 2019
@maxlawton
Copy link
Contributor Author

@maxlawton maxlawton commented Mar 12, 2019

@johbuch Your screenshots indicate that you're using this filter as expected, so I see no obvious reason for its failure.

This filter has been used with success in production, but merged into an outdated revision of the staging branch. It is possible that something changed between the original development and the latest commit to resolve merge conflicts between staging and this branch.

Unless anyone else can verify that this branch does in fact work as expected, I'll try to figure out what has gone wrong. Any clues to a fix would be appreciated.

@kuzmany
Copy link
Member

@kuzmany kuzmany commented Mar 12, 2019

Cannot work because
LeadListRepository > getListFilterExpr is method for filter contacts to segments before Mautic 2.14

Mautic 2.14 and above (after segments refactoring #5755) use SegmentFilterQuery and decorators
https://github.com/mautic/mautic/tree/staging/app/bundles/LeadBundle/Segment

This PR require add new FilterQuery for assets like segments do https://github.com/mautic/mautic/blob/staging/app/bundles/LeadBundle/Segment/Query/Filter/SegmentReferenceFilterQueryBuilder.php

@maxlawton
Copy link
Contributor Author

@maxlawton maxlawton commented Mar 12, 2019

@kuzmany Thanks! I'll refactor accordingly.

@npracht npracht removed this from Code Review (2 required) in Mautic 2 Apr 4, 2019
@maxlawton
Copy link
Contributor Author

@maxlawton maxlawton commented Apr 5, 2019

Hmm. I may have screwed up my merge. The diff looked good locally, but I'm seeing a bunch of unrelated changes here.

@alanhartless Would a rebase and force-push to my branch be unwelcome?

filipelbc and others added 5 commits Apr 5, 2019
The ContactSegmentFilterCrate defaults to 'lead' as the object when
not specified by the filter, but it is the convention to explicitly
specify the object.
Using the ForeignValueFilterQueryBuilder as the filter type appears to
be sufficient for filtering against asset_downloads. No special
Decorator required.
@maxlawton
Copy link
Contributor Author

@maxlawton maxlawton commented Apr 8, 2019

I've rebased on staging and force-pushed, so this PR should be good to go now.

@npracht
Copy link
Member

@npracht npracht commented Apr 8, 2019

@johbuch can you try again please ?

@npracht
Copy link
Member

@npracht npracht commented Apr 8, 2019

@kuzmany can you review code again please ?

johbuch
johbuch previously approved these changes Apr 8, 2019
Copy link
Contributor

@johbuch johbuch left a comment

tested again with mautibox, it works now.
Great job

kuzmany
kuzmany previously approved these changes Apr 8, 2019
Copy link
Member

@kuzmany kuzmany left a comment

👍 works for me

Tested by mautibox.
Code looks ok.

Just replace media/js/app.js by staging version.
It will be regenerate on fly.

@maxlawton
Copy link
Contributor Author

@maxlawton maxlawton commented Apr 8, 2019

@kuzmany Where/when is app.js regenerated? I had to manually build it locally via the console command in order to get those changes to propagate.

@kuzmany
Copy link
Member

@kuzmany kuzmany commented Apr 8, 2019

assets are generate on each version f5504d8
Just commit it from github https://github.com/mautic/mautic/blob/staging/media/js/app.js

@maxlawton maxlawton dismissed stale reviews from kuzmany and johbuch via ca0abf2 Apr 8, 2019
@kuzmany kuzmany self-requested a review Apr 8, 2019
kuzmany
kuzmany approved these changes Apr 8, 2019
@kuzmany kuzmany merged commit df55976 into mautic:staging Apr 13, 2019
2 checks passed
@Woeler Woeler removed this from the 2.16.0 milestone Sep 27, 2019
@Woeler Woeler added this to the 2.15.3 milestone Sep 27, 2019
@wucherpfennig
Copy link

@wucherpfennig wucherpfennig commented Oct 2, 2019

We have been waiting for this functionality for so long... Thank you!

BUT while testing the new filter we discovered that most of the assets do not appear in the dropdown and they are also not accessible via autocompletion.
(We have currently 150+ assets in use)

Does anybody have an idea why?

@npracht
Copy link
Member

@npracht npracht commented Oct 2, 2019

@Woeler or @maxlawton could it be because a pagination or something like that ?
@wucherpfennig can you create a new issue explaining how to reproduce and the reference to that PR ?

Thanks !

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

Successfully merging this pull request may close these issues.

None yet

10 participants