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

[WIP] Public stream archives #8135

Closed
wants to merge 9 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@adnrs96
Collaborator

adnrs96 commented Jan 22, 2018

This PR will track our progress on getting #4817 ready.
The first commit introduces changes so that we don't supply user emails in the rendered content of messages having user mentions. Second commit introduces DB changes in the Stream table so that we can call some of the streams global-public i.e. they are visible to the world without having to login.
Discussion on this is being tracked by https://chat.zulip.org/#narrow/stream/backend/topic/global-public.20streams

@zulipbot zulipbot added size: L size: XL and removed size: L labels Jan 22, 2018

@adnrs96

This comment has been minimized.

Show comment
Hide comment
@adnrs96

adnrs96 Feb 6, 2018

Collaborator

Steps for testing this out after pulling locally:

  • Run provisioning using tools/provision
  • Open up dbshell in a terminal by using the command ./manage dbshell
  • Query the DB for stream names and id's using select id, name from zerver_stream; query.
  • Note down the id for streams you wanna make public. Lets say we have a stream Scotland with id 3
  • Open up another terminal and use it to open shell using the command ./manage shell
  • In this shell execute the following lines
x = [comma separated list of id's which you wanna make web public goes here]
for id in x:
    a = Stream.objects.get(id=id)
    do_change_stream_global_public(a, True)
  • Visit the link of following nature
    http://localhost:9991/archive/streams/<stream_id>/topic/<topic_name>
    Example:
    http://localhost:9991/archive/streams/3/topic/Scotland2
Collaborator

adnrs96 commented Feb 6, 2018

Steps for testing this out after pulling locally:

  • Run provisioning using tools/provision
  • Open up dbshell in a terminal by using the command ./manage dbshell
  • Query the DB for stream names and id's using select id, name from zerver_stream; query.
  • Note down the id for streams you wanna make public. Lets say we have a stream Scotland with id 3
  • Open up another terminal and use it to open shell using the command ./manage shell
  • In this shell execute the following lines
x = [comma separated list of id's which you wanna make web public goes here]
for id in x:
    a = Stream.objects.get(id=id)
    do_change_stream_global_public(a, True)
  • Visit the link of following nature
    http://localhost:9991/archive/streams/<stream_id>/topic/<topic_name>
    Example:
    http://localhost:9991/archive/streams/3/topic/Scotland2
@adnrs96

This comment has been minimized.

Show comment
Hide comment
@adnrs96

adnrs96 Feb 6, 2018

Collaborator

Note: I am temporarily using the global_public DB changes here which are essentially useful for testing this out.
Ideally we will have another PR introducing DB changes for this feature and get that PR merged before this.

Collaborator

adnrs96 commented Feb 6, 2018

Note: I am temporarily using the global_public DB changes here which are essentially useful for testing this out.
Ideally we will have another PR introducing DB changes for this feature and get that PR merged before this.

@timabbott

This comment has been minimized.

Show comment
Hide comment
@timabbott

timabbott Mar 5, 2018

Member

@adnrs96 I took a quick look at this; I'm going to merge the first commit after fixing the tests. I also opened #8588 for a related issue that this reminded me of.

Member

timabbott commented Mar 5, 2018

@adnrs96 I took a quick look at this; I'm going to merge the first commit after fixing the tests. I also opened #8588 for a related issue that this reminded me of.

adnrs96 added some commits Feb 3, 2018

portico: Change CSS class to float-left and float-right.
This is done because the current column-left and column-right were
actually just floating left and right and making use of float-left
and float-right makes more sense. This also helps with the upcoming
public archives feature which will try to include portico content
with main app content.
web_public_streams: Add is_web_public to Stream table.
Also add function do_change_stream_web_public in lib/actions.py
to help in changing a streams web public status.
linter: Make duplicate html tag id detection work with archives.
We modify check-templates to check for duplicate id's in archive
templates and app templates separately. This means we are allowing
app and archive templates to potentially use same id's. This is
needed because we intend to re use some js from the main app and
having same id's help achieve that.
Note: We haven't up until this point actually added archive
templates. This commit is more of a preparatory commit for merging
the basic archive infra.
public_archives: Add basic infra for displaying topics.
We add very basic infra so that we can view any discussion which
happened under a topic of a global public stream without
authorization.
lib/streams.py: Extract get_stream_by_id as a separate function.
We extract get_stream_by_id function out of the body of
access_stream_by_id function to help us access streams for archives.
populate_db: Add a web public stream to dev database.
We flip the Stream "Rome" to be a web public stream. Also we add
attribute is_web_public in various stream dicts and in the
bulk_create_streams function of bulk_create.py responsible for
default stream creation in dev environment.
@timabbott

This comment has been minimized.

Show comment
Hide comment
@timabbott

timabbott May 2, 2018

Member

I merged the first 5 commits, since those look great.

Member

timabbott commented May 2, 2018

I merged the first 5 commits, since those look great.

</p>
{% else %}
<p>
This stream does not exist.

This comment has been minimized.

@timabbott

timabbott May 2, 2018

Member

I think we should make this error message clear about the possibilities: "This stream either does not exist or is not publicly archived to the web.".

@timabbott

timabbott May 2, 2018

Member

I think we should make this error message clear about the possibilities: "This stream either does not exist or is not publicly archived to the web.".

This comment has been minimized.

@timabbott

timabbott May 2, 2018

Member

(And we should tag these strings for translation).

@timabbott

timabbott May 2, 2018

Member

(And we should tag these strings for translation).

</div>
{% elif is_web_public %}
<p>
This topic does not exist.

This comment has been minimized.

@timabbott

timabbott May 2, 2018

Member

I think "No messages have been sent to this topic within the publicly archived stream #%(stream_name)s".

@timabbott

timabbott May 2, 2018

Member

I think "No messages have been sent to this topic within the publicly archived stream #%(stream_name)s".

@@ -9,7 +9,7 @@ def test_non_existant_stream_id(self) -> None:
# Here we use a relatively big number as stream id assumming such an id
# won't exist in the test DB.
result = self.client_get("/archive/streams/100000000/topic/TopicGlobal")
self.assert_json_error(result, 'Invalid stream id')
self.assert_in_success_response(["This stream does not exist."], result)

This comment has been minimized.

@timabbott

timabbott May 2, 2018

Member

Once we merge the main infra commits, I think we can split off a project to make a better error page for this case; I think we'll want to show a list of the public streams in the organization.

@timabbott

timabbott May 2, 2018

Member

Once we merge the main infra commits, I think we can split off a project to make a better error page for this case; I think we'll want to show a list of the public streams in the organization.

@timabbott

This comment has been minimized.

Show comment
Hide comment
@timabbott

timabbott May 2, 2018

Member

I went ahead and merged this, since it doesn't break anything, and it'll be easier to iterate from here, I think. Some notes on immediate follow-ups:

  • Let's fix the error message items I noticed above.
  • We're going to want to make the "left sidebar" equivalent for this feature (which I would consider building as its own page, rather than actually placing it as a sidebar). I would probably start with just building an endpoint to get the metadata and list of topics in a web-public stream (you can borrow some logic from the existing /topics endpoint), sorted by time. This feels pretty independent of whatever we're doing with message rendering.
  • Similarly, we probably will also want to make a /archive homepage for a realm, that just has the realm's banner, and a list of the web-public streams in the organization that you can click on, to get to the per-stream pages.
  • I think we can refactor test_web_public_stream_topic to use the Rome stream; as we add more tests, I think it'll be nicer to be building on top of the existing stream with this setting.
Member

timabbott commented May 2, 2018

I went ahead and merged this, since it doesn't break anything, and it'll be easier to iterate from here, I think. Some notes on immediate follow-ups:

  • Let's fix the error message items I noticed above.
  • We're going to want to make the "left sidebar" equivalent for this feature (which I would consider building as its own page, rather than actually placing it as a sidebar). I would probably start with just building an endpoint to get the metadata and list of topics in a web-public stream (you can borrow some logic from the existing /topics endpoint), sorted by time. This feels pretty independent of whatever we're doing with message rendering.
  • Similarly, we probably will also want to make a /archive homepage for a realm, that just has the realm's banner, and a list of the web-public streams in the organization that you can click on, to get to the per-stream pages.
  • I think we can refactor test_web_public_stream_topic to use the Rome stream; as we add more tests, I think it'll be nicer to be building on top of the existing stream with this setting.

@timabbott timabbott closed this May 2, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment