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

Jump to bottom

[UI]update invalid archive notification #19126

Merged
merged 34 commits into from Jun 7, 2022
Merged

[UI]update invalid archive notification #19126

merged 34 commits into from Jun 7, 2022

Conversation

peterhashair
Copy link
Contributor

@peterhashair peterhashair commented Apr 21, 2022 •
edited

Description:

Fixes: #19011
update invade archive notification caused by config rearchive_reports_in_past_last_n_months limitation

Review

update archive notification
7bb31cc 
update archive notification
@peterhashair peterhashair added this to the 4.10.0 milestone Apr 21, 2022
Peter added 6 commits April 22, 2022 12:07
update condition
1b5dbc2 
update condition
update condition on message
7b14527 
update condition on message
update error
c221460 
update error
Merge branch '4.x-dev' into m-19011
bbc228e
Update _unprocessedSegmentMessage.twig
c36129c 
update unprocessed
update screenshot
b97a893 
update screenshot
@peterhashair peterhashair added 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. labels Apr 26, 2022
sgiehl
sgiehl requested changes Apr 26, 2022
View reviewed changes
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.

Had a look through the code and left some comments. Did not yet do any functional testing

core/Archive/ArchiveInvalidator.php Outdated Show resolved Hide resolved
plugins/SegmentEditor/lang/en.json Outdated Show resolved Hide resolved
plugins/SegmentEditor/SegmentEditor.php Outdated Show resolved Hide resolved
plugins/SegmentEditor/SegmentEditor.php Outdated Show resolved Hide resolved
plugins/SegmentEditor/templates/_unprocessedSegmentMessage.twig Outdated Show resolved Hide resolved
@sgiehl sgiehl removed the Needs Review PRs that need a code review label Apr 26, 2022
Peter added 4 commits April 27, 2022 11:42
update tests and some parts
1fa7d3e 
update tests and some parts
update phpcs error
c1bef04 
update phpcs
update tests
04bfdf7 
update tests
update phpcs
d728f0c 
update phpcs
@peterhashair
Copy link
Contributor Author

peterhashair commented Apr 29, 2022 •
edited

I believe the doc is here, https://matomo.org/wp-admin/post.php?post=775&action=edit&lang=en.

Adding when PR merged

This could be caused by configuration in config.ini.php, the limitation of the archive date setting, under [General] rearchive_reports_in_past_last_n_months, make sure the request period is in the range of the earliest archive date, then rerun the archive process.

update screen shots
001d123 
update screen shots
@peterhashair peterhashair added the Needs Review PRs that need a code review label May 2, 2022
@sgiehl sgiehl modified the milestones: 4.10.0, 4.11.0 May 5, 2022
sgiehl
sgiehl requested changes May 5, 2022
View reviewed changes
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.

The message shown can still be improved. Also the linked FAQ is missing. I've added a couple suggestions to improve it. The UI test will need an update as well. And I was wondering if we can't add another UI test that covers the other case.

plugins/SegmentEditor/lang/en.json Outdated Show resolved Hide resolved
plugins/SegmentEditor/templates/_unprocessedSegmentMessage.twig Outdated Show resolved Hide resolved
@sgiehl sgiehl removed the Needs Review PRs that need a code review label May 5, 2022
Peter added 10 commits May 19, 2022 13:40
@peterhashair peterhashair requested a review from bx80 May 24, 2022 04:12
@peterhashair peterhashair added the Needs Review PRs that need a code review label May 24, 2022
@bx80 bx80 changed the title [UI]update invade archive notification [UI]update invalid archive notification May 27, 2022
bx80
bx80 suggested changes Jun 1, 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 done some functional testing, looks good 👍

There are two failing UI tests related to the segment editor, since this PR makes changes to the segment editor I've pushed an empty commit to rerun the tests in case it's a random failure, if not then we should find out why these tests are failing.

@peterhashair
Copy link
Contributor Author

@bx80 that's wired one, will investigate

@bx80 bx80 removed the Needs Review PRs that need a code review label Jun 1, 2022
@peterhashair
Copy link
Contributor Author

@bx80 I believe the UI test's failure is random.

@peterhashair peterhashair added the Needs Review PRs that need a code review label Jun 3, 2022
bx80
bx80 approved these changes Jun 7, 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.

The UI test fails are unrelated, so this should be good to merge 👍

(Don't forget to update the FAQ 🙂)

@peterhashair peterhashair merged commit a81d553 into 4.x-dev Jun 7, 2022
@peterhashair peterhashair deleted the m-19011 branch June 7, 2022 01:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sgiehl sgiehl sgiehl requested changes

@bx80 bx80 bx80 approved these changes

@justinvelluppillai justinvelluppillai Awaiting requested review from justinvelluppillai

Assignees
No one assigned
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
Milestone
4.11.0
Development

Successfully merging this pull request may close these issues.

When a segment is shown, and no data is there, then show accurate error message when segment won't be archived
3 participants
@peterhashair @sgiehl @bx80