Page MenuHomePhabricator

Make notification default for Conpherence
AbandonedPublic

Authored by chad on Oct 19 2016, 11:31 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Mar 9, 6:03 PM
Unknown Object (File)
Feb 13 2024, 11:37 PM
Unknown Object (File)
Feb 10 2024, 6:13 AM
Unknown Object (File)
Feb 3 2024, 12:34 PM
Unknown Object (File)
Jan 18 2024, 5:18 AM
Unknown Object (File)
Jan 15 2024, 11:49 PM
Unknown Object (File)
Jan 10 2024, 1:17 PM
Unknown Object (File)
Jan 9 2024, 2:23 PM
Subscribers

Details

Reviewers
epriestley
Summary

This flips the default to Conphrence to be "notification" instead of email. To help notify users of new rooms / messages they have gotten, add a new email setting for "added to room" to allow a push notification for new messages and requests.

Test Plan

Add stranger to room, see stranger get email. Make lots of posts, no emails sent to anyone by default.

Diff Detail

Repository
rP Phabricator
Branch
conph-expand-notifications (branched from master)
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 14226
Build 18494: Run Core Tests
Build 18493: arc lint + arc unit

Event Timeline

chad retitled this revision from to Add new setting for when user is added/removed from Conpherence.
chad updated this object.
chad edited the test plan for this revision. (Show Details)
chad added a reviewer: epriestley.
chad planned changes to this revision.Oct 20 2016, 3:33 AM

Are these ever useful? Maybe we could just hide them from all notifications instead? I think they're OK in the rooms themselves but they feel bad to me as "last transactions" in the menu and as mail/notifications in general.

The one counterexample I can come up with is that we had a user add another user to a private room which they shouldn't have been added to. That feels super weak to me as a justification and I'd still rather just always hide these from leaking out of the room, but it's at least some kind of argument.

We can hide these unconditionally by implementing ConpherenceTransaction->shouldHideForMail/shouldHideForFeed and having them always return true on this transaction type (PARTICIPANTS) regardless of who was added/removed.

Also, maybe this message behavior is better?

  • Remove preference in Settings.
  • Default is ALWAYS "notifications".
  • Per-room, opt-in to email for every message.
  • (In the future, add mention email as part of a greater set of improvements to mentions.)

The problem I'm trying to resolve is knowing I've been added to a new room, aka, new user sends me a message. I'd like to get some sort of push notification for that. Should that be Herald instead? With that, I'm cool just getting rid of Conpherence auto-sending emails by default.

Ah. I think you have to do this:

  • When processing transactions (e.g., in applyFinalEffects()) compute added/removed users and save them on something like $this->addedParticipants / $this->removedParticipants (new properties on the Editor object itself).
  • In getMailTo(), examine the properties you wrote (if they were written) to figure out what magic should happen.

Basically, you can't bring $xactions into getMailTo() directly, but you can pass information on the Editor object, since we do guarantee that the same Editor object is used for both calls.

Conceivably you could cheat and just do $this->xactions = $xactions; somewhere, but that's not very nice.

Seems like herald is the less hacky solution?

chad edited edge metadata.
  • half works?
src/applications/conpherence/editor/ConpherenceEditor.php
528โ€“529

this should have triggered lint since array_key_exists takes 2 parameters?

src/applications/conpherence/editor/ConpherenceEditor.php
528โ€“529

We don't currently have lint for parameter counts. It would be possible to implement in at least some cases (including this one) but isn't entirely straightforward.

Do we do policy checks after getMailTo. I seem to be able to generate mail from being added, but not being removed, even with the PHID in the array to be sent mail.

  • seems to work for adding
  • better words and comments
chad retitled this revision from Add new setting for when user is added/removed from Conpherence to Make notification default for Conpherence.Oct 21 2016, 11:09 PM
chad updated this object.
chad edited the test plan for this revision. (Show Details)

Not sure which thing you were thinking about doing with Herald, but at least the specific "you were just added / you were just removed" stuff isn't currently a terribly clean fit for Herald, since most Herald stuff is based only on the state of the object (e.g., if you plug Z123 into the test console, it should ideally be able to run all the rules).

We can't really/easily figure out who was just added/removed with only a Z123. That said, there are some conditions which sort of violate this already (content source, originating inbound email address) and it's usually possible to do it, just feels a little hacky-ish and is hard to test since you can't pass a specific set of transactions to Herald.

That is, overall, Herald acts on the state of the object, not on which changes were made to the object. So "Title" is a Herald field, but "Title was edited" is not, today. We can make it a condition, but if we want users to be able to test it we need to change how the test console works so that you can say "Test what will happen to object X if I apply edits A, B, and C".

The "removed" thing has a related task somewhere -- when any user is removed from subscribers/reviewers/etc, we should really keep them on that final email. I think we'll need to add a mechanism that says "for this email only, you are exempt for permissions checks" since they can often no longer see the object once we do the checks. Let me see if I can dig that up.

T4776 is the general "users should be able to get notified when they get kicked off an object" task.

This is ready for review then.

I guess I can remove the removed code though if it will never work here

I'm planning to implement T4776 for this.

I also don't want to add a new option -- what was your concern with these proposed behaviors that let us not have any settings?

  • Always default to "notify" for messages.
    • Remove existing setting controlling this behavior.
  • Always send email on added/removed.
    • No new option for this behavior.
    • After T10448, we'll probably get an option for this "for free", or nearly-for-free.

oh, that sounded like a many years from now kind of feature. ok.

I think it's pretty manageable to sneak in here somewhere.