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

Add Two New segments #18953

Merged
merged 21 commits into from Mar 26, 2022
Merged

Add Two New segments #18953

merged 21 commits into from Mar 26, 2022

Conversation

peterhashair
Copy link
Contributor

Description:

Fixes: #15985

Add Two New segment Order Revenue (Ecommerce). Revenue left in cart (Ecommerce)

Review

Peter added 3 commits March 16, 2022 14:38
add order revenue
@peterhashair peterhashair changed the title M 15985 Add Two New segments Mar 16, 2022
@peterhashair
Copy link
Contributor Author

Revenue left in the cart still needs a condition on the query, but not sure how to do that. I notice lists of segments maybe needs a update as well https://developer.matomo.org/api-reference/reporting-api-segmentation

Peter added 2 commits March 17, 2022 13:40
update cart
update errors
@sgiehl
Copy link
Member

sgiehl commented Mar 17, 2022

@peterhashair Adding new column classes has a lot bigger impact than you may think. At least the way it is currently done. Having methods to update the column values shouldn't be needed here at all, as that would duplicate code.
I guess the simplest solution would be to overwrite the configureSegments method in the class Piwik\Plugins\Ecommerce\Columns\Revenue and add both segments there. Btw. Revenue left in cart might be selectable using the idGoal IDGOAL_CART

update segment, and add tests
@peterhashair
Copy link
Contributor Author

@sgiehl thanks for the hints, I am still struggling to add an extra condition idGoal =-1, not sure where is the best place to put that, should I make a new Columns and update in getDbDiscriminator function?

@sgiehl
Copy link
Member

sgiehl commented Mar 18, 2022

@peterhashair Unfortunately this one won't be easy. the getDbDiscriminator method is actually only used for the CustomReports plugin. So it won't have any effect on segments. I would need to have a closer look at that as well to provide a detailed solution. But I would start checking if we can use a segment sqlFilter for that.

Peter added 2 commits March 21, 2022 13:10
update query on cart left in cart
update tests
@peterhashair peterhashair added the not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. label Mar 21, 2022
@peterhashair peterhashair added this to the 4.9.0 milestone Mar 21, 2022
@peterhashair
Copy link
Contributor Author

@sgiehl I think somehow it works. but on the bulk tests, TwoVisitsWithCustomVariablesSegmentMatchNONETest::testApi. Somewhere bind map to a wrong field, still trying to fight out why.

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.

@peterhashair You need to "transform" the $matchType parameter. In the segment definition it is ==, but in SQL it would need to be =. You can find something similar in the Segment class of MediaAnalytics plugin for example.

plugins/Ecommerce/Columns/Revenue.php Outdated Show resolved Hide resolved
@peterhashair peterhashair added Needs Review PRs that need a code review c: Documentation For issues related to in-app product help messages, or to the Matomo knowledge base. labels Mar 22, 2022
@peterhashair
Copy link
Contributor Author

peterhashair commented Mar 22, 2022

@sgiehl I think this is working. But not sure where should I document this.

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.

This doesn't work correctly yet.

plugins/Ecommerce/Columns/Revenue.php Outdated Show resolved Hide resolved
plugins/Ecommerce/Columns/Revenue.php Show resolved Hide resolved
@sgiehl sgiehl removed the Needs Review PRs that need a code review label Mar 22, 2022
update support type
@peterhashair peterhashair added the Needs Review PRs that need a code review label Mar 24, 2022
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.

Have you actually checked/tested you changes once? Looking at the failing tests you would have noticed that the ones with the new segments are failing.

plugins/Ecommerce/Columns/Revenue.php Outdated Show resolved Hide resolved
@sgiehl sgiehl removed the Needs Review PRs that need a code review label Mar 24, 2022
@peterhashair
Copy link
Contributor Author

@sgiehl sorry, should be fixed now.

Peter and others added 2 commits March 25, 2022 11:26
@sgiehl
Copy link
Member

sgiehl commented Mar 26, 2022

@peterhashair Seems that is working now. Regarding documentation. The segment listing doesn't need an update, as it is included from demo.matomo.cloud. See https://github.com/matomo-org/developer-documentation/blob/live/docs/4.x/reporting-api-segmentation.md
So once, that was deployed, the list will be updated automatically.

A general documentation around segmentation would also be good to have. Currently it seems there is none, so I have created matomo-org/developer-documentation#630. Feel free to pick that one and start adding a new documentation with the findings you had while developing this issue.

@sgiehl sgiehl merged commit e1eea7a into 4.x-dev Mar 26, 2022
@sgiehl sgiehl deleted the m-15985 branch March 26, 2022 18:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: Documentation For issues related to in-app product help messages, or to the Matomo knowledge base. 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.

New segments for 'Ecommerce Order Revenue' and 'Revenue left in cart'
2 participants