Rename redact_content option to include_content #2650
Conversation
The redact_content option never worked because it read the wrong config section. The PR introducing it (#2301) had feedback suggesting the name be changed to not re-use the term 'redact' but this wasn't incorporated. This reanmes the option to give it a less confusing name, and also means that people who've set the redact_content option won't suddenly see a behaviour change when upgrading synapse, but instead can set include_content if they want to. This PR also updates the wording of the config comment to clarify that this has no effect on event_id_only push. Includes #2422
|
Hmm, given the PRs I wouldn't be surprised if people were using |
|
Well, my thinking was to deliberately not read the old config option because that will mean that anyone who has set that option will suddenly see synapse's behaviour change when they upgrade: if this had worked at some point and then been broken, I'd agree, but it hasn't. We could put in a big warning if you set the old config option? |
|
It has worked though if you set email.redact_content? I honestly don't think people would notice the behaviour changing. I also think we might want to support Is there a reason not to support it? |
|
Oh I see, yes - we could support email.redact_content. I would vote for not supporting push.redact_content though as per above. |
|
Sorry, I don't see the issue with supporting |
|
My issue is that I expect people will have turned it on when they set synapse up however long ago, saw everything worked, forgot all about it and are now going to have push change from under them when they upgrade. |
synapse/config/push.py
Outdated
| # For modern android devices the notification content will still appear | ||
| # because it is loaded by the app. iPhone, however will send a | ||
| # notification saying only that a message arrived and who it came from. | ||
| # | ||
| #push: | ||
| # redact_content: false | ||
| # include_content: false |
There was a problem hiding this comment.
the default for include_content is true. IMO this should be here as well
There was a problem hiding this comment.
as in the commented version should be include_content: true or the description should say the default is true, or both?
There was a problem hiding this comment.
As this method generates the default snippet for the push config I would put include_content: true here (even uncommented)
Having a comment in the description is optional I think. But would not be bad :)
But his is just my "suggestion". I will accept what you think is best here 🙂
There was a problem hiding this comment.
Yeah, this is probably sensible - done.
because we had to wait until the logger was set up
synapse/config/push.py
Outdated
| @@ -33,20 +26,20 @@ def read_config(self, config): | |||
| # 'email'section'. Check for the flag in the 'push' section, and log, | |||
| # but do not honour it to avoid nasty surprises when people upgrade. | |||
| if push_config.get("redact_content") is not None: | |||
| reactor.callWhenRunning(lambda: logger.warn( | |||
| print( | |||
There was a problem hiding this comment.
why not just logger.warn(<msg>) instead of the previous construct
(reactor.callWhenRunning(lambda: logger.warn(<msg>)))?
Shouldn't that be faster as well with the functionality that this message will be part of homeserver.log?
This is especially useful in the case when you run it as service and won't see any console-output
There was a problem hiding this comment.
At this point the logging hasn't been set up and we've not yet daemonized, and using print is consistent with where we do it elsewhere
The redact_content option never worked because it read the wrong config
section. The PR introducing it
(#2301) had feedback suggesting the
name be changed to not re-use the term 'redact' but this wasn't
incorporated.
This reanmes the option to give it a less confusing name, and also
means that people who've set the redact_content option won't suddenly
see a behaviour change when upgrading synapse, but instead can set
include_content if they want to.
This PR also updates the wording of the config comment to clarify
that this has no effect on event_id_only push.
Includes #2422