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.

Sign up for GitHub

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

Jump to bottom

Fix error where no data is shown on campaign with segments and periods #18540

Merged
merged 1 commit into from Jan 7, 2022
Merged

Fix error where no data is shown on campaign with segments and periods #18540

merged 1 commit into from Jan 7, 2022

Conversation

peterhashair
Copy link
Contributor

@peterhashair peterhashair commented Dec 24, 2021 •
edited

Description:

L3: https://innocraft.atlassian.net/browse/L3-199
fix error no data notification.
Show on the campaign with data show.
To reproduce it, need to select a period, and require a segment. The request is that one, in my case.

segment=visitorType==returning&date=2021-09-01,2021-09-15&module=MarketingCampaignsReporting&format=html&action=getGroup&period=range&idSite=1&widget=&showtitle=1&random=2789

The code runs into here

matomo/core/DataAccess/ArchiveSelector.php

Lines 102 to 107 in 1aec00f

if (empty($result['idarchive'])
|| (isset($result['value'])
&& !in_array($result['value'], $doneFlagValues))
) { // the archive cannot be considered valid for this request (has wrong done flag value)
return [false, $visits, $visitsConverted, true, $tsArchived, $value];
}

Review

update condition
33f90a3 
update condition
@peterhashair peterhashair added the Needs Review PRs that need a code review label Dec 24, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Jan 1, 2022

This issue is in "needs review" but there has been no activity for 7 days. ping @matomo-org/core-reviewers

@github-actions github-actions bot added the Stale The label used by the Close Stale Issues action label Jan 1, 2022
@peterhashair peterhashair removed the Stale The label used by the Close Stale Issues action label Jan 4, 2022
bx80
bx80 reviewed Jan 6, 2022
View reviewed changes
Copy link
Contributor

@bx80 bx80 left a comment

Choose a reason for hiding this comment

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

I've been able to recreate the original issue and this fix prevents the 'no data' message from showing.

I might be misunderstanding something here, but If the archive selector is identifying the archive as having an invalid done flag value then should we be accepting a result without an idarchive value?

Would it be better to determine why the archive done flag is incorrect and either fix that if the archive is truly valid for use or if the archive isn't really valid then keep the 'no data' message and prevent the data being shown?

@peterhashair
Copy link
Contributor Author

@bx80 if I am right, I remember the problem was, there are multiple javascript ajax calling this function

matomo/plugins/SegmentEditor/SegmentEditor.php

Line 186 in 9ed7e18

public function onNoData(View $dataTableView)

If one of the ajax returns an empty result, the no data message will show up. In my case is the Campaign Groups, which is no data is expected, but the segment is processed as an archive.

@bx80
Copy link
Contributor

bx80 commented Jan 7, 2022

@peterhashair That makes sense when considering the multiple ajax calls. I can't really think of any scenario where there would be a reason to show the no data (check back later) message when we know that an archive has already been generated for the segment. Just wanted to be sure we're not preventing the message being shown when it is needed. 🙂

@peterhashair peterhashair merged commit db25a2e into 4.x-dev Jan 7, 2022
@peterhashair peterhashair deleted the l3-199-ui-show-error branch January 7, 2022 02:54
@justinvelluppillai justinvelluppillai changed the title fix error no data show on campaing Fix error where no data is shown on campaign with segments and periods Feb 1, 2022
@sgiehl sgiehl mentioned this pull request Nov 30, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bx80 bx80 bx80 approved these changes

Assignees
No one assigned
Labels
Needs Review PRs that need a code review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@peterhashair @bx80