Make default_streams web controllable #817

hackerkid · 2016-05-22T23:10:04Z

Fixes: #665

smarx · 2016-05-22T23:10:06Z

Automated message from Dropbox CLA bot

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

hackerkid · 2016-05-26T14:01:57Z

timabbott · 2016-05-27T08:50:07Z

Playing around with creating a new stream and then trying to add it as a default stream in Chrome, one gets an ugly overlap of the browser's native autocomplete with the default streams autocomplete. We should probably just disable browser autocomplete in this text box.

timabbott · 2016-05-27T08:53:34Z

zerver/views/streams.py

+    streams = do_get_streams(user_profile, include_public=include_public,
+                              include_subscribed=include_subscribed,
+                              include_all_active=include_all_active,
+                              include_default=include_default)


whitespace looks wrong here

timabbott · 2016-05-27T08:54:35Z

Otherwise the UI looks pretty good; thanks for building this @hackerkid!

Can you add some Casper frontend tests for the admin UI for this feature?

hackerkid · 2016-05-27T09:29:49Z

@timabbott Yeah. Sure. Working on it.

hackerkid · 2016-05-29T00:06:02Z

@timabbott Added the tests. Please review.

hackerkid · 2016-05-29T14:16:25Z

@timabbott Fixed the merge conflicts.

timabbott · 2016-05-31T14:55:55Z

@hackerkid can your rebase to resolve the merge conflicts? Looks like just a small conflict with something else I merged yesterday.

Also, I notice that this adds a ton of tab-based whitespace, which we don't allow in the Zulip codebase (and triggers linter errors); you'll want to fix those (and configure your editor to not introduce them!). (Oh, looks like you fixed those in later commits -- can you squash the fixes into the commits that introduced the bugs? In this case I think you can probably just squash everything into a single commit).

I'll take a look at the code as well, so you can fix all the issues in one batch.

timabbott · 2016-05-31T15:02:04Z

frontend_tests/casper_tests/11-admin.js

+    casper.click('#settings-dropdown');
+    casper.click('a[href^="#administration"]');
+    var stream_name = "Scotland";
+    get_suggestions("O");


Why do you pass "0" here?

@timabbott It matches with all the stream names which has the 'O' as a substring (Rome, Scotland, Verona etc). I used 'O' to make sure that it works even if there are multiple suggestions.Capital 'O' is used instead of small 'o' to make sure that the suggestions are not case sensitive. I can change into Scotland if you want.

Ahh, OK, got it-- I misread it as a zero. Can you just add a comment with that explanation of the reasoning on the line above this?

timabbott · 2016-05-31T15:21:51Z

@hackerkid thanks for working on this, it's looking really good; I posted a bunch of comments on little optimizations to make this match the Zulip Python style, cleaner and more performant, but I'm excited about the progress on this!

hackerkid · 2016-06-04T00:16:52Z

@timabbott Made the changes as you suggested. Please check.

timabbott · 2016-06-05T06:41:57Z

Cool, this is looking pretty good! I played around with this and found one more bug:

If you type in a stream name that doesn't exist and hit enter, the admin page reloads (leaving the "default streams) tab

timabbott · 2016-06-05T06:43:19Z

Also, the first of the two commits should include the hunk that deletes make_dict.

hackerkid · 2016-06-06T14:57:38Z

@timabbott Fixed both the issues. Please check.

timabbott · 2016-06-06T21:55:03Z

@hackerkid this looks great! I merged the first commit, and I think there's just one thing this still needs:

Since this adds a new event type, you should add a new test to EventsRegisterTest; often that will end up finding bugs in how once implemented the apply_events code for a feature

Once that's done (and issues are resolved), we can merge this! I'm super excited about this feature.

hackerkid · 2016-06-07T20:54:13Z

@timabbott I have added a test for default_streams event in EventsRegisterTest. Please check.

timabbott · 2016-06-07T23:14:40Z

Test looks great! But I noticed a few more improvements we can make to the templates:

Several of the strings in the template (e.g. "Stream name") aren't marked for translation
Also, while you're tweaking the templates, I think we should style the "Add default stream" control more like the "Add realm emoji" box in the first tab of http://127.0.0.1:9991/#administration
We should add some description for what this is where the text "Add default streams" is currently (also tagged for translation), e.g. "Configure the default streams new users are subscribed to when joining the {{domain}} organization." I just merged a similar description for the "realm emoji" feature if you're looking to borrow code.

(Sorry I didn't catch these in the last round of review!)

timabbott · 2016-06-07T23:35:39Z

Also, a small note, you should add Fixes: #665 to your commit message (usually we do it as its own sentence at the end) so that merging this auto-closes the original issue.,

hackerkid · 2016-06-07T23:38:44Z

@timabbott Sure. Will do that.

hackerkid · 2016-06-09T20:56:19Z

@timabbott Fixed the issues. Please check.

timabbott · 2016-06-09T21:14:33Z

Can you rebase on top of master so the Travis CI tests will rerun post-rebase?

hackerkid · 2016-06-09T21:25:07Z

@timabbott Done 👍

Fixes: zulip#665

timabbott · 2016-06-09T22:28:15Z

Awesome, thanks @hackerkid, I've merged this. I'm very excited -- this feature is a key part of the Zulip 2016 roadmap :)

hackerkid · 2016-06-09T22:30:01Z

@timabbott I am also excited to see this getting merged. 😄

timabbott reviewed May 27, 2016
View reviewed changes

hackerkid force-pushed the make-default-streams-web-configurable branch 3 times, most recently from 37939c1 to cbe10bd Compare May 29, 2016 00:05

hackerkid force-pushed the make-default-streams-web-configurable branch 2 times, most recently from fb9c1b7 to 88163b8 Compare May 29, 2016 14:02

hackerkid force-pushed the make-default-streams-web-configurable branch 3 times, most recently from a35a92a to 636a8ce Compare May 30, 2016 23:29

timabbott reviewed May 31, 2016
View reviewed changes

hackerkid force-pushed the make-default-streams-web-configurable branch from f127206 to 887298c Compare June 3, 2016 23:55

hackerkid force-pushed the make-default-streams-web-configurable branch 2 times, most recently from 37e1d6f to a64de6d Compare June 6, 2016 14:53

hackerkid force-pushed the make-default-streams-web-configurable branch 4 times, most recently from b7a15ea to d5344c0 Compare June 7, 2016 20:42

hackerkid force-pushed the make-default-streams-web-configurable branch 4 times, most recently from d8e742c to b85bc65 Compare June 9, 2016 20:43

hackerkid force-pushed the make-default-streams-web-configurable branch from b85bc65 to 8345eca Compare June 9, 2016 21:23

hackerkid force-pushed the make-default-streams-web-configurable branch from 8345eca to d9b373e Compare June 9, 2016 21:26

Make default_streams web controllable.

28b2682

Fixes: zulip#665

hackerkid force-pushed the make-default-streams-web-configurable branch from d9b373e to 28b2682 Compare June 9, 2016 21:52

timabbott closed this Jun 9, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make default_streams web controllable #817

Make default_streams web controllable #817

hackerkid commented May 22, 2016 •

edited

smarx commented May 22, 2016

hackerkid commented May 26, 2016 •

edited

timabbott commented May 27, 2016

timabbott May 27, 2016

timabbott commented May 27, 2016

hackerkid commented May 27, 2016

hackerkid commented May 29, 2016

hackerkid commented May 29, 2016

timabbott commented May 31, 2016 •

edited

timabbott May 31, 2016

hackerkid May 31, 2016

timabbott May 31, 2016

timabbott commented May 31, 2016

hackerkid commented Jun 4, 2016

timabbott commented Jun 5, 2016

timabbott commented Jun 5, 2016

hackerkid commented Jun 6, 2016 •

edited

timabbott commented Jun 6, 2016

hackerkid commented Jun 7, 2016

timabbott commented Jun 7, 2016 •

edited

timabbott commented Jun 7, 2016

hackerkid commented Jun 7, 2016

hackerkid commented Jun 9, 2016

timabbott commented Jun 9, 2016

hackerkid commented Jun 9, 2016

timabbott commented Jun 9, 2016

hackerkid commented Jun 9, 2016

Make default_streams web controllable #817

Make default_streams web controllable #817

Conversation

hackerkid commented May 22, 2016 • edited

smarx commented May 22, 2016

hackerkid commented May 26, 2016 • edited

timabbott commented May 27, 2016

timabbott May 27, 2016

Choose a reason for hiding this comment

timabbott commented May 27, 2016

hackerkid commented May 27, 2016

hackerkid commented May 29, 2016

hackerkid commented May 29, 2016

timabbott commented May 31, 2016 • edited

timabbott May 31, 2016

Choose a reason for hiding this comment

hackerkid May 31, 2016

Choose a reason for hiding this comment

timabbott May 31, 2016

Choose a reason for hiding this comment

timabbott commented May 31, 2016

hackerkid commented Jun 4, 2016

timabbott commented Jun 5, 2016

timabbott commented Jun 5, 2016

hackerkid commented Jun 6, 2016 • edited

timabbott commented Jun 6, 2016

hackerkid commented Jun 7, 2016

timabbott commented Jun 7, 2016 • edited

timabbott commented Jun 7, 2016

hackerkid commented Jun 7, 2016

hackerkid commented Jun 9, 2016

timabbott commented Jun 9, 2016

hackerkid commented Jun 9, 2016

timabbott commented Jun 9, 2016

hackerkid commented Jun 9, 2016

hackerkid commented May 22, 2016 •

edited

hackerkid commented May 26, 2016 •

edited

timabbott commented May 31, 2016 •

edited

hackerkid commented Jun 6, 2016 •

edited

timabbott commented Jun 7, 2016 •

edited