-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Add a Segment Management Page #23885
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
base: 5.x-dev
Are you sure you want to change the base?
Conversation
13932db to
597f770
Compare
adcf8af to
6fb66bd
Compare
fc0cb9b to
e353f4f
Compare
| tr:nth-last-child(1 of .dataTable_row-group) td { | ||
| border-bottom: 3px solid @theme-color-background-tinyContrast !important; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| interface PiwikHelperGlobal { | ||
| escape(text: string): string; | ||
| redirect(params?: any); | ||
| getCurrentQueryStringWithParametersModified(newparams: string); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ This function already exists (and is useful!) but it is now available outside of the piwikHelper.
| public function addMeasurableItem($menuName, $url, $order = 50, $tooltip = false, $icon = false) | ||
| { | ||
| $this->addItem('CoreAdminHome_MenuMeasurables', $menuName, $url, $order, $tooltip); | ||
| $this->addItem('CoreAdminHome_MenuMeasurables', $menuName, $url, $order, $tooltip, $icon); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ This argument already exists and it is false by default. I only made it available in a higher level.
| .sparklineEvolution { | ||
| color: black; | ||
| font-size: 12px; | ||
| white-space: nowrap; | ||
| } | ||
|
|
||
| .sparklineEvolution-positive { | ||
| color: green; | ||
| } | ||
|
|
||
| .sparklineEvolution-negative { | ||
| color: red; | ||
| } | ||
|
|
||
| .sparklineEvolution_icon { | ||
| margin-right: 1em; | ||
| vertical-align: middle; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
|
||
| if (Piwik::isUserHasWriteAccess($idSite)) { | ||
| $menu->addMeasurableItem('Goals_Goals', $this->urlForAction('manage', array('idSite' => $idSite)), 40); | ||
| $menu->addMeasurableItem('Goals_Goals', $this->urlForAction('manage', array('idSite' => $idSite)), 15); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ Goals item should be higher in the Administration Menu. It is more obvious now that we add a "Segments" item.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You better should not change that. If you want to add something after Goals, just use a higher number.
Decreasing the number here has the potential to break the order how our plugins are listed there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. The PO asked for Goals to be higher, because he wasn’t happy with the current plugin order.
| } | ||
|
|
||
| function addTooltip(element, title) { | ||
| $('.ui-tooltip').remove(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ Avoid bug, when several tooltips are stack. It happens when DOM that trigger the tooltip is destroyed, for example when clicking on a star, the star is rebuild.
| updateStarSegmentTooltip($segment, segment); | ||
| function updateStarredSegment(segment, isError = false) { | ||
| starCallbackList.forEach(function(callback) { | ||
| callback(segment, isError); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ When the state of a segment change, it changes automatically for the segmentEditor and the segmentPage in the same time.
| window.SegmentEditorPanel = { | ||
| addTooltip, | ||
| askToDeleteSegment, | ||
| closePanel, | ||
| getDeleteSegmentTitle, | ||
| getEditSegmentTitle, | ||
| getIsUserCanEditSegment, | ||
| getSegmentFromId, | ||
| normalizeSearchString, | ||
| onSegmentsStarChange, | ||
| openPanel, | ||
| openEditFormGivenIdSegment, | ||
| togglePanel, | ||
| toggleStarredSegment, | ||
| triggerStarAnimation, | ||
| updateStarSegmentTooltip, | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ Theses functions are also useful for the SegmentPage and the SegmentPanel, so we make it public to avoid duplication and inconsistency.
| "AddANDorORCondition": "Add %s condition", | ||
| "AddNewSegment": "Add new segment", | ||
| "AreYouSureDeleteSegment": "Are you sure you want to delete this segment?", | ||
| "AddNewSegment": "Create new segment", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ For consistency with Form / Funnels / Reports, we changed the label from "Add" to "Create" and displayed a "+" icon.
| display: flex; | ||
| width: 16px; | ||
| height: 16px; | ||
| .segmentAction { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ Reduce selector complexity to improve reusability
| </ul> | ||
|
|
||
| {% if authorizedToCreateSegments %} | ||
| <a tabindex="4" class="add_new_segment btn">{{ 'SegmentEditor_AddNewSegment'|translate }}</a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ <a> tag without href attribute is an anchor and is not accessible.
| <svg xmlns="http://www.w3.org/2000/svg" width="16" height="16" viewBox="0 0 24 24"> | ||
| <path stroke="black" stroke-width="3" fill="none" d="M9.153 5.408C10.42 3.136 11.053 2 12 2c.947 0 1.58 1.136 2.847 3.408l.328.588c.36.646.54.969.82 1.182.28.213.63.292 1.33.45l.636.144c2.46.557 3.689.835 3.982 1.776.292.94-.546 1.921-2.223 3.882l-.434.507c-.476.557-.715.836-.822 1.18-.107.345-.071.717.001 1.46l.066.677c.253 2.617.38 3.925-.386 4.506-.766.582-1.918.051-4.22-1.009l-.597-.274c-.654-.302-.981-.452-1.328-.452-.347 0-.674.15-1.329.452l-.595.274c-2.303 1.06-3.455 1.59-4.22 1.01-.767-.582-.64-1.89-.387-4.507l.066-.676c.072-.744.108-1.116 0-1.46-.106-.345-.345-.624-.821-1.18l-.434-.508c-1.677-1.96-2.515-2.941-2.223-3.882.293-.941 1.523-1.22 3.983-1.776l.636-.144c.699-.158 1.048-.237 1.329-.45.28-.213.46-.536.82-1.182l.328-.588Z"/> | ||
| </svg> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ Star, in a future PR, will be handled with a background image to reduce the page weight and improve reusability
58c8392 to
ff37dbc
Compare
9b0745b to
6939639
Compare
| var tooltip = $(root).parents('.matomo-widget').tooltip('instance'); | ||
| if (tooltip) { | ||
| tooltip.disable(); | ||
| } | ||
| window.SegmentEditorPanel.updateStarSegmentTitle($starButton, segment); | ||
| if (tooltip) { | ||
| tooltip.enable(); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That does not fully seem to work as expected. If I hover an unstared segment it displays Add to Starred segments for this website.. Clicking on the icon then stars the segment. But hovering the icon afterwards again still shows the same tooltip. Instead the tooltip content should change.
caddoo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done a review over everything biggest thing missing are tests testing the interactions.
Also you don't need to have the TagManager change as part of this.
| } | ||
| const $segment = $button.closest('tr'); | ||
| const idSegment = $button.attr('data-star'); | ||
| window.SegmentEditorPanel.toggleStarredSegment($segment, idSegment); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed when this API call fails the item still gets starred and the UI shows the user it's starred, however there is a read error message at the top.
Is it easy to only update the UI if the call is successful?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We chosed optimistic UI so the interface seems reactive.
But, it the star should go back to its place with a red/error animation if there is an error.
So there is definitely something to check here on my side.
Thanks for the catch!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
plugins/SegmentEditor/Menu.php
Outdated
| $idSite = Request::fromRequest()->getIntegerParameter('idSite', 0); | ||
| if (Piwik::isUserHasWriteAccess($idSite)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels weird, it's a general link to the segment editor, not site specific.
Also from the implementation of the segment editor it doesn't seem like write access is required to view it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You’re right. There is 2 options here:
- Display the link to everyone, because all users have access to this page. But, all users cannot use it to "manage" segment so it could be weird to have a "view" page in an administration plugin
- Display the link to all users that can create segment. So it will depends on the segment configuration and the user level.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Done!
I removed the access control because it is linked to the idSite, and the idSite is not mandatory in the administration part. Since all users have access to the segment management page, it should be the right behaviour.
| Piwik::checkUserHasViewAccess($this->idSite); | ||
| } | ||
|
|
||
| public function manageSegments(): string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sgiehl Just a question for you this controller being open is generally fine right? (permission wise).
When anon user has no access then all controllers aren't accesible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, if anonymous user are authorized this page is accessible., but they can’t see all segments.
The same way they can apply some segments on the Dashboard.
| * @license https://www.gnu.org/licenses/gpl-3.0.html GPL v3 or later | ||
| */ | ||
|
|
||
| describe("SegmentManagementPageTest", function () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you expand this test suite with some interaction tests as well to make sure everything always works and doesn't regress.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I wanted to have feedback and reviews before taking this task.
|
@tzi Is there a reason why you have built this using controller / view / template / plain javascript, instead of implementing the whole thing in a (more dynamic) vue component.? That would allow a lot more flexible handling. |
I do agree that Vue should be the long term target for this kind of page. |
Add 'All Visits at the top' Reorder segments: fixed > selected > starred > others Add highlighted styles to fixed and selected segments Allow star/unstar segments Reorder data Add view, edit, and delete actions Synchronize UI between the editor panel and the management page Align number to the right Add right management Use translation for the page description Implement the search segment Add tooltip on view action Add a create segment button Add sparkline UX Meeting feedback Add a menu entry into the Administration menu + colored evolution Fix range selection Update evolution tooltip Add manage segment button Remove tooltip on sparkline when using a custom range Reorder administration menu and add an outlink icon Use generic table tooltip instead of custom ones Fix PHPCS Fix PHP errors Update the Scheduled Reports help box about segment Move JavaScript in a separate file and fix a bug when we unstar selected segment Bugfix: not all users are allowed to create segments
sgiehl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked through most parts of the code (not really at the css parts, though) and left a couple of comments with suggestions, questions and concerns that came to my mind when looking through it.
I only performed some happy tests in the UI yet, but might be good to also do some edge case tests.
And another word on the vue component topic. I still believe it would have been more beneficial to directly do it. While trying to have things reusable and to also reuse existing this is a good manner, I think that should only be done for things that are expected to remain.
Now we will end up in having two things (Segment Editor & Management Page), that both rely on old code. If we want to migrate that to vue js, we will have to migrate both at the same time, or keep a lot legacy code until both parts are migrated.
If the management page would have been implemented as a new component with according stores and things like that directly, we might not have been able to reuse existing things, but the new code would have been cleaner and reusable as well. Then it might have been easier to migrate the old stuff step by step by switching it to new code where already possible.
So would be awesome to at least consider that for other stuff we start working on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change seems unexpected
|
|
||
| if (Piwik::isUserHasWriteAccess($idSite)) { | ||
| $menu->addMeasurableItem('Goals_Goals', $this->urlForAction('manage', array('idSite' => $idSite)), 40); | ||
| $menu->addMeasurableItem('Goals_Goals', $this->urlForAction('manage', array('idSite' => $idSite)), 15); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You better should not change that. If you want to add something after Goals, just use a higher number.
Decreasing the number here has the potential to break the order how our plugins are listed there.
| { | ||
| public function configureAdminMenu(MenuAdmin $menu) | ||
| { | ||
| $idSite = Request::fromRequest()->getIntegerParameter('idSite', 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should not use a static default value here. A site with id 1, might not exist on an instance if it has been removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch.
How can I create a link to the Dashboard that works for an environment where there is possibly no site selected?
The default idSite value in the code is always 0, but this doesn’t work.
Should I retrieve the idSite visible for the current user and take the first one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, something like that. Not sure if we already have something similar in place somewhere 🤔
| foreach ($view->segmentList as $index => &$segment) { | ||
| $segment['fixed'] = $segment['fixed'] ?? false; | ||
| $segment['selected'] = $segment['definition'] === $this->currentSegmentDefinition; | ||
| $segment['dashboardUrl'] = '?module=CoreHome&action=index&idSite=' . $this->idSite . '&period=' . $this->period . '&date=' . $this->strDate . '&segment=' . urlencode($segment['definition']); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might cause problems when attaching the segment to the main url query parameters. The UI might not be able to handle it correctly if it then differs between that and the parameters in hash part.
If you follow such a link, it e.g. is not really possible anymore to switch back to the All Visits segment, as the segment part remains in the query parameters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting! How can we create this kind of link in this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can't do that properly in the backend, and that might not be needed all. A dynamic link with javascript should be quite easy to add. braodcast.buildReportingUrl() might be something that can be used.
| $deleteButton.attr('data-state', 'disabled'); | ||
| } | ||
| window.SegmentEditorPanel.updateStarSegmentTitle($starButton, segment); | ||
| $editButton.attr('title', window.SegmentEditorPanel.getEditSegmentTitle(segment, canEdit)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That actually does not work properly. The fetched segment contains enable_only_idsite, but as a string value. As the method getEditSegmentTitle checks that in a boolean compare it results to be true all the time, causing the site specific message to be shown for global segments as well.
| } | ||
| window.SegmentEditorPanel.updateStarSegmentTitle($starButton, segment); | ||
| $editButton.attr('title', window.SegmentEditorPanel.getEditSegmentTitle(segment, canEdit)); | ||
| $deleteButton.attr('title', window.SegmentEditorPanel.getDeleteSegmentTitle(segment, canEdit)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same for getDeleteSegmentTitle
| $allVisitsSegment = [ | ||
| 'definition' => '', | ||
| 'name' => Piwik::translate('SegmentEditor_DefaultAllVisits'), | ||
| 'fixed' => true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This currently causes the all visits segment to always be highlighted, even if another segment is currently chosen. It looks a bit weird if two segments are highlighted at the same time.
Is there any specific reason why the All Visits row is always highlighted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because it's what the PO asked for
| <a href="{{ segment.dashboardUrl }}" class="table-action icon-show" title="{{ 'SegmentEditor_SeeDashboardForThisSegment'|translate }}"></a> | ||
| {% endif %} | ||
| {% if segment.idsegment is defined %} | ||
| <button class="table-action icon-edit" data-edit-segment="{{ segment.idsegment }}"></button> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way how segments are edited might not be perfect from a usability perspective. When you click edit it opens that segment editor. That might be fine, but if you change it, you will always have to click "Save & Apply", which will trigger the segment to be selected and the page to be reloaded.
In case there are a lot segments the page might take long to reload, which actually only the data of that one segment might require a reload.
But guess that's a trade of we might need to live with atm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that’s what the PO asked for. It's a pragmatic solution to have a smaller PR and a shorter development time.
| </svg> | ||
| </button> | ||
| {% endif %} | ||
| </td> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be awesome to also have some icons that show directly if a segment is a shared one and if it's shared across users and / or sites.
But that could be added in a later version as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, because it's hard to know for now.
Where should we trace this kind of idea?





Description
UX-326
We propose the introduction of a new page in Matomo, titled "Segments."
This page will serve as a combined reporting and management tool to make it easy to view (and edit) all segments for a website, and also provide a clean dashboard to view All segment data at once.
Features
If needed, a tooltip explain why we cannot do an action. If it is a global segment, the tooltip precise that starring/unstarring it will affect all websites.
If we choose to edit a segment, it opens the segmentEditorPanel
If we choose to delete a segment, it opens the segmentEditorPanel and trigger a new modal that ask confirmation before deleting it.
Data Synchronization with the Panel
Starring/Unstarring will reorder segments without reload. The Page and the Panel are always synchronized.
Menus
We added an
outlinkicon to the administration because this link goes back to the Dashboard. We choose this behaviour because the website / date / segment selector are needed and not available in the administration layout.Plugins
This PR will have impact on plugins UI screenshots:
Checklist
Review