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

Allow reports to specify a column/metadata other than "label" that uniquely identifies a row #20009

Merged
merged 16 commits into from May 11, 2023

Conversation

diosmosis
Copy link
Member

@diosmosis diosmosis commented Nov 14, 2022

Description:

Problem: A report in a plugin needs row evolution to work to provide as much value as possible, but the report does not have unique labels (but does have another column whose values (IDs of a log table) are unique). The current solution would result in incorrect rows being compared.

How this PR works:

This PR adds a property to Report, $rowIdentifier, which reports can use to specify a column other than 'label'. LabelFilter now uses this property to determine what row in a report to filter.

There are additionally some changes to what label value is in the row evolution popover. In the rowaction JavaScript, we build the display label based on the values displayed in the UI and send this with the row evolution request for the twig template to use. This is required if labelColumn is used and there is no data selected. In this case, there is no way to know what the display value should be based on the data.

Note: this PR will stay as a draft until there is a plugin available that uses the feature.

refs PG-2667

Review

…iquely identifies a row, and use this in row evolution/label filter for reports where label is not unique.
@diosmosis diosmosis added this to the 5.0.0 milestone Nov 14, 2022
@diosmosis diosmosis marked this pull request as draft November 14, 2022 23:52
@github-actions
Copy link
Contributor

github-actions bot commented Dec 2, 2022

If you don't want this PR to be closed automatically in 28 days then you need to assign the label 'Do not close'.

@github-actions github-actions bot added the Stale The label used by the Close Stale Issues action label Dec 2, 2022
@diosmosis diosmosis added the Do not close PRs with this label won't be marked as stale by the Close Stale Issues action label Dec 2, 2022
@diosmosis diosmosis marked this pull request as ready for review December 2, 2022 10:23
@diosmosis
Copy link
Member Author

Moving this out of draft, but at present the only way to test it would be via crash analytics reports, where some reports have non-unique labels.

@github-actions github-actions bot removed the Stale The label used by the Close Stale Issues action label Dec 3, 2022
@mattab mattab added the 5.0.0 label Jan 4, 2023
@tsteur tsteur added the Needs Review PRs that need a code review label Apr 4, 2023
@tsteur
Copy link
Member

tsteur commented Apr 4, 2023

That's a nice new addition @diosmosis that will likely come in handy also for other use cases in the future.
Is there a possibility to add some test maybe?
FYI there's now also a merge conflict and a "unused use statement".

@sgiehl did you have otherwise any thoughts there?

@diosmosis diosmosis removed the Needs Review PRs that need a code review label Apr 6, 2023
…o adding to list of entity ID parameters, allow label filter row ID to be specified by DataTable (for test), and add test for LabelFilter change
@diosmosis
Copy link
Member Author

Was able to add a test for the LabelFilter test. I don't think it's easily doable to add a test for the UI changes, but they will be covered in the crash analytics UI tests.

@diosmosis diosmosis added the Needs Review PRs that need a code review label May 9, 2023
@tsteur
Copy link
Member

tsteur commented May 10, 2023

Any chance we could get this reviewed soon?

Copy link
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

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

Left some suggestions for code improvements, otherwise this looks good to me.

config/global.php Show resolved Hide resolved
core/API/DataTableManipulator/LabelFilter.php Outdated Show resolved Hide resolved
@sgiehl sgiehl removed the Needs Review PRs that need a code review label May 10, 2023
@sgiehl sgiehl removed the Do not close PRs with this label won't be marked as stale by the Close Stale Issues action label May 10, 2023
@diosmosis diosmosis added the Needs Review PRs that need a code review label May 11, 2023
@sgiehl sgiehl merged commit 4437960 into 5.x-dev May 11, 2023
17 of 19 checks passed
@sgiehl sgiehl deleted the row-identifier-property branch May 11, 2023 12:04
diosmosis added a commit that referenced this pull request Jun 1, 2023
sgiehl added a commit that referenced this pull request Jun 5, 2023
* apply #20009 to 4.x-dev

* updated expected test file

* fix failing UI test

---------

Co-authored-by: Stefan Giehl <stefan@matomo.org>
@sgiehl sgiehl added the not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. label Jul 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants