@nina-py opened this Pull Request on November 20th 2020 Contributor

Description:

Turning the multi_server_environment setting on now hides the following
settings from the Administration -> Settings -> General settings page:

  • Archiving settings
  • Email server
  • Trusted Matomo hostname
  • Update settings

...so that users cannot set these separately on different instances of Matomo. Assuming I got these right, are there any other settings that should be disabled when multi_server_environment is turned on?

Fixes #14390.

Review

  • [ ] Functional review done
  • [ ] 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
@tsteur commented on November 22nd 2020 Member

Makes sense @nina-py

Might need to add a note in config/global.ini.php for multi_server_environment that this flag doesn't need to be enabled when the config file is on a shared filesystem like NFS or EFS.

@nina-py commented on November 22nd 2020 Contributor

Thanks @tsteur, added a note to the global config file.

@nina-py commented on November 30th 2020 Contributor

Hi @tsteur , is there anything else that needs to be done in this PR? Or are you guys just way too busy at the moment following the 4.0.x release?

@tsteur commented on November 30th 2020 Member

@nina-py at the moment we're indeed quite busy with the Matomo 4 release. @sgiehl can you do another review maybe?

@nina-py commented on December 14th 2020 Contributor

Hi @sgiehl, in addition to the changes you requested earlier, I have updated a UI screenshot (an extra comment for the setting about half-way through the screenshot): https://builds-artifacts.matomo.org/matomo-org/matomo/4.x-dev/44409/UIIntegrationTest_admin_diagnostics_configfile.png. CI build in progress.

As far as I can see, the other failures in those two jobs are not related to the changes introduced in this PR. I've also rebased the branch. Please review.

@nina-py commented on December 14th 2020 Contributor

@sgiehl, just to follow up on the most recent CI build for this PR - the updated screenshot matches but there is one failure elsewhere with the following:

There was 1 failure:

1) Piwik\Tests\Integration\ReleaseCheckListTest::test_screenshotsStoredInLfs

Some Screenshots are not stored in LFS: tests/UI/expected-screenshots/UIIntegrationTest_admin_diagnostics_configfile.png

Failed asserting that an array is empty.

/home/travis/build/matomo-org/matomo/tests/PHPUnit/Integration/ReleaseCheckListTest.php:180

Do I need to do anything about this one?

@sgiehl commented on December 14th 2020 Member

@nina-py We are storing our screenshots in Git LFS to not waste space in this repo. You may need to install Git LFS to update the the srceenshots properly

@nina-py commented on December 14th 2020 Contributor

Thank you @sgiehl, I think I got this right this time around - the commit diff shows

Git LFS file not shown 

but the build is yet to run.

@nina-py commented on December 14th 2020 Contributor

Thank you @sgiehl for all your help with this one!

This Pull Request was closed on December 15th 2020
Powered by GitHub Issue Mirror