Add custom visitor cookie timeout setting #563
Conversation
Add possibility to set custom a custom cookie timeout in the Matomo config variable as described in matomo-org#562
There was a problem hiding this comment.
Hi @tomper00 . Thank you for taking the time to create this PR. I think it looks pretty good, but there are a few changes I'd like to request.
- Could you please update the tests that are broken by your changes? If you need help with the screenshots or any other parts, just let me know.
- Could you please create a migration to add your new fields to existing tags?
- Could you please add some validation to the
customCookieTimeOutfield. The typing handles some validation, but I think a minimum value would be a good idea to at least prevent 0 and negative values.
|
@mattab This customer provided PR is looking really good and I'd like to merge it after a few additions/adjustments. Do the new fields look alright to you and do the labels/wording look alright? |
Updated failed tests Added validation to customCookieTimeOut Added tag migration script (hopefully got this right=
Add num range validator
tests/System/expected/test___TagManager.exportContainerVersion_site_default_container.xml
Outdated
Show resolved
Hide resolved
tests/System/expected/test_webContext__TagManager.getAvailableVariableTypesInContext.xml
Outdated
Show resolved
Hide resolved
Based on great feedback from snake14
Make it easier to configure by using full days instead of seconds
393 days instead of 33955200 seconds
….xml Fix tests with new defaults
|
@snake14 Great feedback, thanks for the help! |
There was a problem hiding this comment.
@tomper00 Things are looking good. I only had one more requested change after your change from seconds to days. My only other thought is that some customers might want more granularity than a day, but what you've got should work.
Fix default value in update script to match full days!
|
Good point with the granularity, but it's probably good enough for now. I updated the update script default value now as well, thanks again for the feedback! |
Changes to doUpdate after comment from @snake14
Another fix, not reading comments good enough..
There was a problem hiding this comment.
@tomper00 left few textual changes, rest looks good and thanks for the PR 👍
Submitting suggestions for better translations Co-authored-by: Altamash Shaikh <altu9594@gmail.com>
submitting translation improvements. Co-authored-by: Altamash Shaikh <altu9594@gmail.com>
|
@AltamashShaikh thanks for helping out, your changes are submitted now. |
Fix tests to match new translations
Force rebuild since tests failed for suspicious reasons
Add possibility to set custom a custom cookie timeout in the Matomo config variable as described in #562
Description:
This will add a new setting in the Matomo Config variable where you can manually set the timeout in seconds for the pk_id cookie.
Review