Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

notifications: Add stream email notifications. #9181

Closed

Conversation

shubhamdhama
Copy link
Member

@shubhamdhama shubhamdhama commented Apr 22, 2018

This completes #7788.
@timabbott FYI.
I've fixed some minor and major merge conflicts so it's better to see a diff between original and these commits.

@shubhamdhama shubhamdhama changed the title notifications: Add stream email notifications. [WIP]notifications: Add stream email notifications. Apr 22, 2018
@shubhamdhama shubhamdhama force-pushed the notifications-sarah-work-email branch 4 times, most recently from 6f89657 to d460a4a Compare April 25, 2018 17:10
@shubhamdhama shubhamdhama changed the title [WIP]notifications: Add stream email notifications. notifications: Add stream email notifications. Apr 25, 2018
@timabbott
Copy link
Sponsor Member

@shubhamdhama can you rebase this? It's like 1000 commits behind master :)

@shubhamdhama shubhamdhama force-pushed the notifications-sarah-work-email branch from d460a4a to 31d480f Compare May 31, 2018 06:07
@shubhamdhama
Copy link
Member Author

@timabbott Resolved conflicts, though I'm not completely familiar with changes to how to test whether it does its job properly or not?

@showell
Copy link
Contributor

showell commented Jul 6, 2018

FYI @shubhamdhama and I chatted about how to test this, and he's going to work on that today and ask questions on chat if any come up.

@timabbott
Copy link
Sponsor Member

@shubhamdhama do you have an update on the status of the work you and Steve discussed? Maybe something for us to chat about in person at the retreat tomorrow.

@showell
Copy link
Contributor

showell commented Jul 11, 2018

I'm guessing this just kinda got lost in the shuffle. My advice is for Tim to just look over the shoulder of @shubhamdhama as he manually tests this, just to make sure all the new email niceness we have in dev works as advertised and to give any handy tips for manual testing in general.

@shubhamdhama
Copy link
Member Author

Sure we can discuss this tommorow!! BTW here is the link to the screenshots of the mails: https://chat.zulip.org/#narrow/stream/6-frontend/subject/testing.20emails/near/609440

@timabbott timabbott force-pushed the notifications-sarah-work-email branch from b3b55bb to ecbeedb Compare July 12, 2018 08:24
@timabbott
Copy link
Sponsor Member

@shubhamdhama and I did a rebase/split of this into a longer commit series, fixing a few bugs and several more things that were confusing about this PR. I then merged this as the commit series ending with de2ec8a.

Huge thanks to @sarahcstringer and @shubhamdhama for all their work on this!!!

Here are a couple follow-up projects we should probably do:

  • Fix messages[0].header.plain to be more readable; I think we can use the stream_name value that was passed alongside trigger.
  • Can we improve how test_end_to_end_missedmessage_hook does the database setup to use the do_foo function rather than hacking up the object?
  • @rishig I bet there's some documentation updates we need to make following this being merged; can you make sure those happen?

@timabbott timabbott closed this Jul 14, 2018
timabbott pushed a commit that referenced this pull request Aug 8, 2018
This test refactor makes the subscription/stream settings changes use standard
APIs and thus be easier to follow (and more robust to subtle re-fetching bugs).

This is a follow-up to #9181.
ljagielski2 pushed a commit to ljagielski2/zulip that referenced this pull request Aug 18, 2018
This test refactor makes the subscription/stream settings changes use standard
APIs and thus be easier to follow (and more robust to subtle re-fetching bugs).

This is a follow-up to zulip#9181.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants