-
Notifications
You must be signed in to change notification settings - Fork 295
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
Analytics settings db migration #8123
Analytics settings db migration #8123
Conversation
Build files for 12a0638 have been deleted. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the hard work here, @zutigrm.
I have left a few comments. Let's update the test cases and QAB accordingly as well.
Also, let's fix the merge conflicts.
Thanks!
Thanks for the feedback @nfmohit . PR and QAB updated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for addressing my feedback, @zutigrm. I have left a few comments, mostly suggestions. Please let me know what you think, thanks!
@nfmohit Thanks, PR updated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Aleksej! This looks perfect now.
I have pointed out a few very very minor changes in the test suite. I would've just gone ahead and applied them myself as they're so minor, but I'm not sure about one usage that I've pointed out here.
Apologies for the back and forth and thank you for your patience!
@nfmohit Thank you, I forgot for a moment I still had these included. PR updated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Brilliant work, thank you @zutigrm.
Some very minor changes needed, so minor that I've applied them myself. I hope that's okay with you.
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you @zutigrm !
…2-analytics-db-migration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work, @nfmohit 🎖️
…2-analytics-db-migration.
…2-analytics-db-migration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@@ -0,0 +1,212 @@ | |||
<?php | |||
/** | |||
* Migration for 1.123.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Arguably this should use the n.e.x.t
placeholder, but seeing as the class name uses the underscore-separated format with the version hardwired, I think we can keep it as-is.
a04a247
into
feature/singular-analytics-module
🚨 Before you merge
Please ensure you've read the requirements for this issue and implications to consider before merging here.
Summary
Addresses issue:
analytics
toanalytics-4
#8082Relevant technical choices
1_119_0
since it might go out with current release, rather than next onePR Author Checklist
Do not alter or remove anything below. The following sections will be managed by moderators only.
Code Reviewer Checklist
Merge Reviewer Checklist