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

Casper test for subscription button created by announcement on creation of new stream. #509

Closed
wants to merge 23 commits into from

Conversation

anindya
Copy link
Contributor

@anindya anindya commented Mar 10, 2016

casper test for bug discussed here. Uses a secondary email-id (hamlet@zulip.com) to check Subscribe and Unsubscribe functionality from the button created by notification bot.

@smarx
Copy link

smarx commented Mar 10, 2016

Automated message from Dropbox CLA bot

@anindya, it looks like you've already signed the Dropbox CLA. Thanks!

if realm_user_dict['email'] == user_profile.email:
notifications.append(internal_prep_message(settings.NOTIFICATION_BOT,
"private",
realm_user_dict['email'], "", msg_self))
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this breaks the previous case where realm_user_dict['email'] in principals since that will no longer continue?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The msg_self will be the message sent to the creators stream and if the email is in principals, then it will get a regular notification of the stream creation.

@timabbott
Copy link
Sponsor Member

Hi @anindya, thanks for working on this!

I posted one small comment, but this branch is pretty confusing now:

  • It seems to have lost the actual casper test?
  • It still contains the code to setup the secondary test user, even though this user isn't used.
  • I don't understand the purpose of the change in streams.py -- it seems to add a new announcement private message, whereas what we'd discussed previously was that the whole private message codepath is to be removed, we should instead change the test setup code in populate_db.py to make sure a notifications_stream is created for the automated tests, not to change the actual notifications code.

@anindya
Copy link
Contributor Author

anindya commented Mar 13, 2016

Hi @timabbott, the code here is sending notification to everyone including the creator of the stream. This way, the public stream of every user will have the notification of the stream creation.
If that works for us, then we can write the tests for announcement button by changing the 04-subscriptions and adding the functionality of testing the button from the users(creators) own home page.

@anindya
Copy link
Contributor Author

anindya commented Mar 13, 2016

@timabbott , I misunderstood some part of the code in stream.py file. I am going over the streams.py to understand it once again and I am looking at the test file creation directly by changing populate_db.py. I will make clean commit and work after that.

@@ -32,7 +33,7 @@
from six.moves import range

settings.TORNADO_SERVER = None

DEFAULT_NOTIFICATION_STREAM_NAME = "notifications"
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should just use Realm.DEFAULT_NOTIFICATION_STREAM_NAME here -- no need to make up a new stream name.

…d by default and used as notification stream. Modified frontend test file 12 to make a new a new stream and test the announcement button by narrowing to zulip stream.
@brainwane
Copy link
Contributor

@anindya Once you resolve the conflicts and rebase, will this pull request be ready to be reviewed again?

If there's anything about this you want to discuss in realtime, feel free to stop by https://zulip.tabbott.net anytime -- including during our realtime office hours on 3 October.

@timabbott timabbott closed this Feb 17, 2017
@timabbott
Copy link
Sponsor Member

This PR never implemented the relevant issue, and seems unlikely to ever be finished. If you start working on this again, feel free to open a new PR @anindya!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants