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

Show notification when there is raw data but no data #17380

Merged
merged 16 commits into from Apr 6, 2021

Conversation

flamisz
Copy link
Contributor

@flamisz flamisz commented Mar 23, 2021

Description:

fixes #16790

Review

  • Functional review done
  • Potential edge cases thought about (behavior of the code with strange input, with strange internal state or possible interactions with other Matomo subsystems)
  • Usability review done (is anything maybe unclear or think about anything that would cause people to reach out to support)
  • Security review done see checklist
  • Code review done
  • Tests were added if useful/possible
  • Reviewed for breaking changes
  • Developer changelog updated if needed
  • Documentation added if needed
  • Existing documentation updated if needed

@flamisz flamisz added not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Needs Review PRs that need a code review labels Mar 23, 2021
@flamisz flamisz self-assigned this Mar 23, 2021
@flamisz flamisz added this to the 4.3.0 milestone Mar 23, 2021
@flamisz
Copy link
Contributor Author

flamisz commented Mar 23, 2021

The original issue says:

For now only apply when "no segment" is select (report shows all visits)

This solution works with segments as well but easy to disable if we don't want to support yet. Is it ok if it works with segments @tsteur?

Update:

the Live.getLastVisitsDetails does not work properly with segments, so I disable it for now

@flamisz flamisz added Needs Review PRs that need a code review and removed Needs Review PRs that need a code review labels Mar 23, 2021
@flamisz flamisz added Needs Review PRs that need a code review and removed Needs Review PRs that need a code review labels Mar 24, 2021
@flamisz flamisz requested a review from sgiehl March 25, 2021 01:05
@sgiehl sgiehl removed the Needs Review PRs that need a code review label Mar 25, 2021
@flamisz flamisz added the Needs Review PRs that need a code review label Mar 26, 2021
@flamisz flamisz requested a review from sgiehl March 26, 2021 02:10
@sgiehl sgiehl removed the Needs Review PRs that need a code review label Mar 26, 2021
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.

Looks go to me now. We could theoretically try to add a new UI test that shows this notification. But that might not be too easy. UI tests by default run the archiving, so by default reports would be generated when you add a visit for today. Guess we could maybe use an empty fixture and send a tracking request for today in the UI test itself or something like that.

@sgiehl sgiehl removed the Needs Review PRs that need a code review label Mar 30, 2021
@flamisz
Copy link
Contributor Author

flamisz commented Mar 30, 2021

I tried to add that test, but couldn't make it work. I tried this inside the _spec.js file: (I tried with form params as well instead of params in the URL)

       const trackerUrl = config.piwikUrl + "matomo.php?action_name=&idsite=1&url=http://piwik.net/test/url";

        await request({
            method: 'POST',
            uri: trackerUrl,
        });

        await page.goto(pageUrl);
        expect(await page.screenshot({ fullPage: true })).to.matchImage('show_notification_when_only_raw_data_exists');

But there is no new log in the db...
Any thoughts on this @sgiehl?
I found one example here:

it('should have a working cron archiving process', async function () {
// track one action
const trackerUrl = config.piwikUrl + "latestStableInstall/piwik.php?";
await request({
method: 'POST',
uri: trackerUrl,
form: {
idsite: 1,
url: 'http://piwik.net/test/url',
action_name: 'test page',
},
});

EDIT: I talked to @tsteur about this, he showed me a couple of things in the tests, so I try to make it work.

@flamisz
Copy link
Contributor Author

flamisz commented Mar 30, 2021

@tsteur and @sgiehl I figured what was the problem and what is the solution. Even when I called the tracker it didn't work, because when we load the page that triggers the archiving, so I need to add this to test, and I don't need any extra tracker call this way.:

       testEnvironment.configOverride.General = {
            browser_archiving_disabled_enforce: '1',
            enable_browser_archiving_triggering: '0',
        };
        testEnvironment.optionsOverride = {
            enableBrowserTriggerArchiving: '0',
        };

Only one thing that came up now. We show this notification only if today's date is in the date range. I couldn't find a way to mock the today in the UI tests. Are we happy with it if the test uses today every time? It means we can have some false negatives, when the tests run around midnight.

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.

Should be good to merge now

@flamisz flamisz merged commit 2c30dc8 into 4.x-dev Apr 6, 2021
@flamisz flamisz deleted the 16790-only-raw-data-yet branch April 6, 2021 01:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

When a period has no data, but raw data, show a message
4 participants