feat: Show Building if a segment is new or updated but not yet rebuilt #14431

driskell · 2025-01-04T12:59:11Z

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?	🟢

Description

When creating a new segment with filters, it displays immediately "No Contacts". This can be confusing to new users as it does not indicate there is an action waiting to build the new segment. Additionally, once build begins, you will see "123 Contacts" - however, the build may not be finished yet. The only way a user can know it is finished is to keep refreshing the page until the count does not change for a minute. It's not really easy to know if it's completed.

Similarly, when editing a segment, for example if you realised you captured too many due to a typo, after saving you just see the same contact count. There is no indication that a rebuild is waiting that might substantially change the count (even potentially reducing it to "No Contacts"). Neither is there indication of progress or when it completes. So again you are left refreshing the page waiting for it to stop changing. In cases where the change is 0 it is then even more difficult as you cannot know if it just didn't get around to rebuilding yet or if it did rebuild but it was 0 change because of an error in your filters.

This PR adds in additional statuses, bringing the list to:

Building - Displayed for a new segment that has filters which is not yet built
No Contacts - Displayed for a segment with filters that has been successfully built at least once since creation or last modification
Building (X Contacts) - Displayed for a segment with filters that has been modified since it was last built
X Contacts - Displayed for a segment with filters that has been successfully built at least once since creation or last modification

Screenshot for a new segment:

After mautic:segments:update passes over it:

After it is modified by a user:

This then returns to the previous screen after mautic:segments:update passes over it.

📋 Steps to test this PR:

Open this PR on Gitpod or pull down for testing locally (see docs on testing PRs here)
Create a new segment without filters. It will display No Contacts.
Create a new segment with filters. It will display Building.
Run mautic:segments:update. It will now display No Contacts or X Contacts depending on the filters.
Edit the segment's filters. It will display either Building or Building (X Contacts) depending on the previous filters.
Run mautic:segments:update. It will now display No Contacts or X Contacts depending on the filters.

I did deprecate the Mautic\LeadBundle\Entity\LeadList::initializeLastBuiltDate as we need to allow null last built date for new segments. Previously it would be set to a last built date of now for a new segment which was incorrect. The field was always nullable though so it's hard to say how bad a BC change this would be. I left the method in place as it was public, and marked it @deprecated.

My sponsoring organisation here is Other Media

driskell · 2025-01-04T13:00:30Z

I have some other work pending that hopefully I can follow up with later. Specifically there is one where I made mautic:segments:update accept a parameter to say only update new or modified segments. Running this as a new cron allows the Building labels to be rapidly removed and provides a massive benefit to users as they don't need to wait ages for the update to eventually reach their segment - in parallel we specifically run updates for new or modified.

driskell · 2025-01-04T13:00:42Z

Will check tests in some time. Looks like I was gonna add test for the class changes in the AJX and forgot! Will look at it when I can but welcome feedback in the interim

codecov · 2025-01-04T13:07:55Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.65%. Comparing base (8558601) to head (36d9f12).
Report is 4 commits behind head on 6.x.

Additional details and impacted files

@@             Coverage Diff              @@
##                6.x   #14431      +/-   ##
============================================
- Coverage     63.65%   63.65%   -0.01%     
- Complexity    34640    34642       +2     
============================================
  Files          2277     2277              
  Lines        103625   103629       +4     
============================================
- Hits          65965    65963       -2     
- Misses        37660    37666       +6

Files with missing lines	Coverage Δ
...p/bundles/LeadBundle/Controller/AjaxController.php	`20.72% <100.00%> (+1.55%)`	⬆️
app/bundles/LeadBundle/Entity/LeadList.php	`97.12% <ø> (-0.10%)`	⬇️

... and 1 file with indirect coverage changes

driskell · 2025-01-06T10:35:36Z

Fixed the tests and added missing CSS classes test.
Not sure what the CS Fixer failure is as it seems completely unrelated to the code so possible an existing test failure.

andersonjeccel · 2025-01-06T12:34:28Z

Hey @driskell, could you rebase your branch and change the target to 6.x?

5.2 is receiving only bug fixes at this time

driskell · 2025-01-06T13:58:44Z

Hey @driskell, could you rebase your branch and change the target to 6.x?

5.2 is receiving only bug fixes at this time

Ah is there no 5.3 due then? (I targeted 5.x not 5.2)

I rebased to 6.x anyway but it looks like it cleanly applies to both with exception of 1 line in the AjaxController so easy to cherry if there was going to be a 5.3 and it was deemed useful.

andersonjeccel · 2025-01-06T15:43:49Z

@driskell We're moving all new enhancements and features to 6.x without 5.3. As the release calendar is many months late, Mautic will skip some versions until we get up to date :)

andersonjeccel · 2025-01-06T15:58:40Z

@driskell Re. the failing tests, could you check PHUnit?

driskell · 2025-01-06T18:04:30Z

I think bogus - looks like a random fail due to time or something and the baseline never was changed. I’ll push again to force rerun as perhaps it ran it on the push before I changed the base and didn’t rerun

driskell · 2025-01-06T18:18:36Z

@andersonjeccel Looks good now!

btw do you know is there a way for me to get onto the development slack? I think the register is locked for external users at the moment.

driskell · 2025-01-07T10:08:41Z

I think the needs-rebase tag can be removed now

andersonjeccel

Before running command with contacts added manually to dynamic segment

Recently set filter:

After running update command:

It seems to be working and it's good for UX

andersonjeccel · 2025-01-07T13:21:04Z

@driskell btw, could you ping me in the Mautic Slack? I'm there as aj

driskell · 2025-01-07T13:23:11Z

@driskell btw, could you ping me in the Mautic Slack? I'm there as aj

I tried to register but I think searching forums there's an invite limit that might be reached. If I click https://mau.tc/slack-invite it only allows Mautic domains for the email registration.

andersonjeccel · 2025-01-07T13:30:24Z

@driskell Asked the lead to update, could you try again using the same link?

andersonjeccel · 2025-01-07T13:31:16Z

@escopecz Would you like to finish code review?

escopecz · 2025-01-08T15:24:19Z

app/bundles/LeadBundle/Controller/AjaxController.php

+        /** @var ListModel $model */
+        $model    = $this->getModel('lead.list');


This model is already available via controller action DI and can be removed

Suggested change

/** @var ListModel $model */

$model = $this->getModel('lead.list');

escopecz · 2025-01-08T15:37:26Z

app/bundles/LeadBundle/Controller/AjaxController.php

+        if ($building) {
+            $html = $this->translator->trans(
+                'mautic.lead.list.building',
+                ['%count%' => $leadCount]
+            );
+        } else {
+            $html = $this->translator->trans(
                'mautic.lead.list.viewleads_count',
                ['%count%' => $leadCount]
-            ),
+            );
+        }


We can avoid some duplicated code here:

$message = $building ? 'mautic.lead.list.building' : 'mautic.lead.list.viewleads_count'; $html = $this->translator->trans($message, ['%count%' => $leadCount]);

escopecz · 2025-01-08T15:46:49Z

app/bundles/LeadBundle/Resources/views/List/_list_row.html.twig

+        {% if (item.getFilters() is not empty and (item.lastBuiltDate is null or (item.dateModified is not null and item.dateModified.timestamp >= item.lastBuiltDate.timestamp))) %}
+            <span size="sm" class="label label-info col-count" data-id="{{ item.id }}">
+                <a href="{{ path('mautic_contact_index', {'search': 'mautic.lead.lead.searchcommand.list'|trans ~ ':' ~ item.alias}) }}"
+                    data-toggle="ajax">{{ 'mautic.lead.list.building'|trans({'%count%': leadCounts[item.id]}) }}</a>
+            </span>
+        {% else %}
+            <span size="sm" class="label label-gray col-count" data-id="{{ item.id }}">
+                <a href="{{ path('mautic_contact_index', {'search': 'mautic.lead.lead.searchcommand.list'|trans ~ ':' ~ item.alias}) }}"
+                    data-toggle="ajax">{{ 'mautic.lead.list.viewleads_count'|trans({'%count%': leadCounts[item.id]}) }}</a>
+            </span>
+        {% endif %}


I can see this is the same condition as in the controller. As it uses only values that are already known to the LeadList entity, let's make a method there:

public function needsRebuild(): bool { return count($leadList->getFilters()) && ( null === $leadList->getLastBuiltDate() || $leadList->getDateModified() >= $leadList->getLastBuiltDate() ); }

Then you can call it in the controller as well as here and avoid the duplicated HTML code:

Suggested change

{% if (item.getFilters() is not empty and (item.lastBuiltDate is null or (item.dateModified is not null and item.dateModified.timestamp >= item.lastBuiltDate.timestamp))) %}



<a href="{{ path('mautic_contact_index', {'search': 'mautic.lead.lead.searchcommand.list'|trans ~ ':' ~ item.alias}) }}"

data-toggle="ajax">{{ 'mautic.lead.list.building'|trans({'%count%': leadCounts[item.id]}) }}</a>



{% else %}



<a href="{{ path('mautic_contact_index', {'search': 'mautic.lead.lead.searchcommand.list'|trans ~ ':' ~ item.alias}) }}"

data-toggle="ajax">{{ 'mautic.lead.list.viewleads_count'|trans({'%count%': leadCounts[item.id]}) }}</a>



{% endif %}



<a href="{{ path('mautic_contact_index', {'search': 'mautic.lead.lead.searchcommand.list'|trans ~ ':' ~ item.alias}) }}"

data-toggle="ajax">{{ (item.needsRebuild() ? 'mautic.lead.list.building' : 'mautic.lead.list.viewleads_count')|trans({'%count%': leadCounts[item.id]}) }}</a>



I'm not sure if this is possible in Twig but you get the idea.

yep should be able to call item.needsRebuild()

escopecz · 2025-01-08T15:48:59Z

app/bundles/LeadBundle/Translations/en_US/messages.ini

+mautic.segment.last_built="Last built"
+mautic.segment.last_built_duration_ms="Build time (milliseconds)"
+mautic.segment.not_built="Not yet built"


Where is this being used?

Indeed it needs removing, snuck in from a previous POC I think where it had an additional status before the build begin, but in review on our side it felt unnecessary to expose the fact that builds run on cron at specific times

driskell · 2025-01-08T16:06:03Z

Thanks for the review @escopecz - all need some changes to clear them up and will aim to look at that in next couple days.

driskell force-pushed the segment-rebuild-status branch 2 times, most recently from 61cf652 to 44c6718 Compare January 6, 2025 09:27

andersonjeccel self-requested a review January 6, 2025 12:35

driskell force-pushed the segment-rebuild-status branch from 44c6718 to 1d9a78d Compare January 6, 2025 13:57

driskell changed the base branch from 5.x to 6.x January 6, 2025 13:57

feat: Show Building if a segment is new or updated but not yet rebuilt

36d9f12

driskell force-pushed the segment-rebuild-status branch from 1d9a78d to 36d9f12 Compare January 6, 2025 18:10

driskell mentioned this pull request Jan 6, 2025

Date Modified populated on create #14433

Open

andersonjeccel approved these changes Jan 7, 2025

View reviewed changes

andersonjeccel added code-review-needed PR's that require a code review before merging pending-test-confirmation PR's that require one test before they can be merged ux-review-passed php Pull requests that update Php code and removed needs-rebase PR's that need to be rebased labels Jan 7, 2025

andersonjeccel requested a review from escopecz January 7, 2025 13:30

escopecz requested changes Jan 8, 2025

View reviewed changes

escopecz added the pending-feedback PR's and issues that are awaiting feedback from the author label Jan 8, 2025

escopecz added this to the 6.0 milestone Jan 8, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Show Building if a segment is new or updated but not yet rebuilt #14431

feat: Show Building if a segment is new or updated but not yet rebuilt #14431

driskell commented Jan 4, 2025 •

edited

Loading

driskell commented Jan 4, 2025

driskell commented Jan 4, 2025 •

edited

Loading

codecov bot commented Jan 4, 2025 •

edited

Loading

driskell commented Jan 6, 2025

andersonjeccel commented Jan 6, 2025

driskell commented Jan 6, 2025

andersonjeccel commented Jan 6, 2025

andersonjeccel commented Jan 6, 2025

driskell commented Jan 6, 2025

driskell commented Jan 6, 2025 •

edited

Loading

driskell commented Jan 7, 2025

andersonjeccel left a comment

andersonjeccel commented Jan 7, 2025

driskell commented Jan 7, 2025

andersonjeccel commented Jan 7, 2025

andersonjeccel commented Jan 7, 2025

escopecz Jan 8, 2025

escopecz Jan 8, 2025

escopecz Jan 8, 2025 •

edited

Loading

driskell Jan 8, 2025

escopecz Jan 8, 2025

driskell Jan 8, 2025

driskell commented Jan 8, 2025

		/** @var ListModel $model */
		$model = $this->getModel('lead.list');

feat: Show Building if a segment is new or updated but not yet rebuilt #14431

Are you sure you want to change the base?

feat: Show Building if a segment is new or updated but not yet rebuilt #14431

Conversation

driskell commented Jan 4, 2025 • edited Loading

Description

📋 Steps to test this PR:

driskell commented Jan 4, 2025

driskell commented Jan 4, 2025 • edited Loading

codecov bot commented Jan 4, 2025 • edited Loading

Codecov Report

driskell commented Jan 6, 2025

andersonjeccel commented Jan 6, 2025

driskell commented Jan 6, 2025

andersonjeccel commented Jan 6, 2025

andersonjeccel commented Jan 6, 2025

driskell commented Jan 6, 2025

driskell commented Jan 6, 2025 • edited Loading

driskell commented Jan 7, 2025

andersonjeccel left a comment

Choose a reason for hiding this comment

andersonjeccel commented Jan 7, 2025

driskell commented Jan 7, 2025

andersonjeccel commented Jan 7, 2025

andersonjeccel commented Jan 7, 2025

escopecz Jan 8, 2025

Choose a reason for hiding this comment

escopecz Jan 8, 2025

Choose a reason for hiding this comment

escopecz Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

driskell Jan 8, 2025

Choose a reason for hiding this comment

escopecz Jan 8, 2025

Choose a reason for hiding this comment

driskell Jan 8, 2025

Choose a reason for hiding this comment

driskell commented Jan 8, 2025

driskell commented Jan 4, 2025 •

edited

Loading

driskell commented Jan 4, 2025 •

edited

Loading

codecov bot commented Jan 4, 2025 •

edited

Loading

driskell commented Jan 6, 2025 •

edited

Loading

escopecz Jan 8, 2025 •

edited

Loading