Looks ok 👍

Though maybe it would be better to change 'segment-change-confirm' to 'segment-definition-change-confirm' and 'ChangingSegmentConfirmation' to 'ChangingSegmentDefinitionConfirmation' to point out this functionality concerns segment definition change?

@adaqus thanks for the comment. I've changed the name/id as you suggested.

@mattab is the message for the confirmation fine or should it be changed? If not, I would merge this PR.

Review:

• let's add a UI test for this use case, so it won't regress
• change text to: You're about to change the segment definition. Your analytics reports for this new segment won't be available until the reports are re-processed. It may take a few hours for reports data to show for this segment. Proceed anyway?

Ok. I've changed the text and added a ui test:
http://builds-artifacts.piwik.org/piwik/piwik/master/19275/SegmentSelectorEditorTest_update_confirmation.png

just realised we can make things a bit more clear and reduce some possible confusion from the wording:

• if( ! Rules::isRequestAuthorizedToArchive() || globalConfig.General.browser_archiving_disabled_enforce ) then print the message Your analytics reports for this new segment won't be available until the reports are re-processed. It may take a few hours for reports data to show for this segment..
• Otherwise (ie. case where reports will be processed in the browser), maybe write Your analytics reports for this new segment will be re-processed the next time you request them, which may take some time.

(the if statement logic reproduces some sub logic of Rules::isArchivingDisabledFor() and can be added as a method there)

After this change LGTM and feel free to merge!

@mattab I've tried to implement your suggestion. Not sure if I got that correct. Would you please have a look at cd05517
If it's correct, feel free to merge,

Note: We need to update the UI test screenshot afterwards

Review:

• just saw again that our segments can individually be set to be pre-processed only, so let's make the message variant depending on the segment.auto_archive as well
• after clicking Proceed Anyway -> No, then editing the segment and clicking Save again, nothing happens (no popup, no save).
• If users often edit segments, the message may get a bit too much... I'm wondering whether:
  • we could hide the message completely when browser archiving is available for this segment?
  • or maybe we could add a check box [ ] Hide this notice in the future that would be saved in the user preference?

just saw again that our segments can individually be set to be pre-processed only, so let's make the message variant depending on the segment.auto_archive as well

I'm a bit confused now. What will happen when the segment setting and the config setting differ? Will a segment not be processed at all if I choose real time processing for the segment, but have a config setting enforcing not to do it? (or vice versa)

What will happen when the segment setting and the config setting differ? Will a segment not be processed at all if I choose real time processing for the segment, but have a config setting enforcing not to do it? (or vice versa)

If a segment is set to real time processing, but browser archiving (real time processing) is disabled in the UI, and enforced via General.browser_archiving_disabled_enforce=1 in config, then the segment will be processed in the next core:archive run (via crontab).

FYI: if a segment is set to "real time processing" but the browser archiving is disabled and enforced, then the data for the segment cannot be processed, and we show this popup:

New attempt ;) Confirmation should now also work as expected (more than once). Message now also depends on segments auto-archive option, but will only be displayed if browser archiving is not available.
I've also implemented an checkbox to hide the message in the future.

If a segment is set to real time processing, but browser archiving (real time processing) is disabled in the UI, and enforced via General.browser_archiving_disabled_enforce=1 in config, then the segment will be processed in the next core:archive run (via crontab).

fyi: after double checking what I wrote yesterday I realised it wasn't actually working this way. Created bug report: #10307 and PR: #10309

Ok I'll have a look at your PR. What about this one? Did you have another look? Can we merge?