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

Segment categories #9544

Merged
merged 92 commits into from
Jan 27, 2021
Merged

Segment categories #9544

merged 92 commits into from
Jan 27, 2021

Conversation

bkomel
Copy link
Contributor

@bkomel bkomel commented Dec 29, 2020

Q A
Branch? features
Bug fix? no
New feature? yes
Deprecations? no
BC breaks? no
Automated tests included? no
Related user documentation PR URL
Related developer documentation PR URL
Issue(s) addressed

Description:

This feature allows users to add new categories of type 'segment' and sort segments (lead lists) into this categories. When listing the segments it is possible to filter them based on the category.

Steps to test this PR:

  1. Load up this PR
  2. Create a new category of type 'segment' and name it Segment Category 1.
  3. Go to segments and create a new segment named Segment 1. Select the category created in previous step.
  4. Create a new uncategorized segment named Segment 2.
  5. In Segments click on the 'Segment Category Filter' in the toolbar and select Segment Category 1.
  6. Now only Segment 1 is listed.

@cla-bot
Copy link

cla-bot bot commented Dec 29, 2020

Thank you for your contribution! We require all contributors to sign our Contributor License Agreement, and we do not have a record of your signature on file. In order for us to review and merge your code, please head over to https://www.mautic.org/contributor-agreement and complete the form. There may be a short delay while the team add you as a contributor - please be patient :). Any problems contact the Product Team on Slack (get an invite at https://mautic.org/slack). CLA has not been signed by @bkomel.

@npracht npracht added categories Anything related to categories feature A new feature for inclusion in minor or major releases segments Anything related to segments needs-automated-tests PR's that need automated tests before they can be merged ready-to-test PR's that are ready to test labels Dec 29, 2020
@npracht npracht added this to the 3.3 milestone Dec 29, 2020
Copy link
Member

@kuzmany kuzmany left a comment

Choose a reason for hiding this comment

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

Looks good. Please add migrations

@RCheesley
Copy link
Sponsor Member

@cla-bot check

@cla-bot cla-bot bot added the cla-signed The PR contributors have signed the contributors agreement label Jan 6, 2021
Copy link
Member

@dennisameling dennisameling left a comment

Choose a reason for hiding this comment

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

Code is looking great, thanks @bkomel! I tested the migration locally and it works without issues. Nice that you even included a down() function 🚀 Since this has been tested by multiple people already, I'll go ahead and merge this PR now 😊

@dennisameling dennisameling removed the pending-feedback PR's and issues that are awaiting feedback from the author label Jan 27, 2021
@dennisameling dennisameling merged commit c0c6add into mautic:features Jan 27, 2021
@dennisameling dennisameling added the needs-documentation PR's that need documentation before they can be merged label Jan 27, 2021
@dennisameling
Copy link
Member

dennisameling commented Feb 13, 2021

This PR is breaking the API library tests unfortunately: https://github.com/mautic/api-library/actions/runs/562027599

Will have a closer look tomorrow.

Note to self: I think it's caused by the fact that it adds a dependency to the CategoryModel in the ListModel. The CategoryModel is dependent on the BrowserStack for getting the current request 🤯 - will have a closer look tomorrow, brain is tired 😅

UPDATE: Bingo! Found a solution. CategoryModel's constructor shouldn't try to get the current request already in the constructor, since it's null at that point. I think I've come across this before, so I want to fix this in more places once and for all. To be continued.

@dennisameling
Copy link
Member

Fixed in #9667

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
categories Anything related to categories cla-signed The PR contributors have signed the contributors agreement feature A new feature for inclusion in minor or major releases needs-documentation PR's that need documentation before they can be merged segments Anything related to segments
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants