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

Render Stream Descriptions From the Backend #11284

Closed
wants to merge 1 commit into from
Closed

Render Stream Descriptions From the Backend #11284

wants to merge 1 commit into from

Conversation

Hypro999
Copy link
Member

Read about the issue here: #11272

@timabbott
Copy link
Sponsor Member

I think you also need to patch create_stream_if_needed to render the descriptions. And now that I think about it, also do something with populate_db (bulk_create_streams, which creates streams in a hacky way optimized for fast development environment setup, should call the render function too).

@Hypro999
Copy link
Member Author

also do something with populate_db (bulk_create_streams, which creates streams in a hacky way optimized for fast development environment setup, should call the render function too).

Ah, this was what I was looking for. Despite making appropriate changes for the "Update the function that creates a stream" part , my tests weren't passing and after a good debugging session, I knew that it was due to the test data, I just didn't know where to modify it. Thanks for the tip!

@timabbott
Copy link
Sponsor Member

Cool, let me know once you have this fixed (no hurry).

@Hypro999
Copy link
Member Author

@timabbott, I think that the backend portion for this feature is about done. Could you please have a look? P.S. I made more commits than required so that it would be easy to review, when you think that the code looks good, we can squash them accordingly.

@timabbott
Copy link
Sponsor Member

@Hypro999 it looks like you resolve the migrations conflicts incorrectly and renumbered a bunch of existing migrations. That does not work at all. Instead, you want to be renumbering your new migration.

Otherwise, this looks good to me.

@Hypro999
Copy link
Member Author

In retrospect, I realize that renumbering that way was a bad idea. @timabbott, I've fixed it. Is there anything else that might need to be changed? Also, thanks for reviewing!

This commit does the following three things:
    1. Update stream model to accomodate rendered description.
    2. Render and save the stream rendered description on update.
    3. Render and save stream descriptions on creation.

Further, the stream's rendered description is also sent whenever the
stream's description is being sent.
@Hypro999
Copy link
Member Author

Hypro999 commented Feb 1, 2019

I squashed the commits, rebased with master and updated the commit message.

@timabbott
Copy link
Sponsor Member

This looks great @Hypro999! I made a few changes:

  • s/rendered_value/rendered_descriptionin the event format; I think there's a reasonable argument for either approach, but I felt thatrendered_description` would feel more rational to folks writing clients like the mobile apps.
  • I applied this change to handle our data import tools:
diff --git a/zerver/data_import/import_util.py b/zerver/data_import/import_util.py
index d843116..fe23713 100644
--- a/zerver/data_import/import_util.py
+++ b/zerver/data_import/import_util.py
@@ -328,6 +328,7 @@ def build_stream(date_created: Any, realm_id: int, name: str,
         name=name,
         deactivated=deactivated,
         description=description,
+        # We don't set rendered_description here; it'll be added on import
         date_created=date_created,
         invite_only=invite_only,
         id=stream_id)
diff --git a/zerver/lib/import_realm.py b/zerver/lib/import_realm.py
index d88268b..e6b0f61 100644
--- a/zerver/lib/import_realm.py
+++ b/zerver/lib/import_realm.py
@@ -22,7 +22,7 @@ from zerver.lib.timestamp import datetime_to_timestamp
 from zerver.lib.export import DATE_FIELDS, \
     Record, TableData, TableName, Field, Path
 from zerver.lib.message import do_render_markdown, RealmAlertWords
-from zerver.lib.bugdown import version as bugdown_version
+from zerver.lib.bugdown import version as bugdown_version, convert as bugdown_convert
 from zerver.lib.upload import random_name, sanitize_name, \
     guess_type, BadImageError
 from zerver.lib.utils import generate_api_key, process_list_in_batches
@@ -742,6 +742,11 @@ def do_import_realm(import_dir: Path, subdomain: str, processes: int=1) -> Realm
     # Stream objects are created by Django.
     fix_datetime_fields(data, 'zerver_stream')
     re_map_foreign_keys(data, 'zerver_stream', 'realm', related_table="realm")
+    # Handle rendering of stream descriptions for import from non-Zulip
+    for stream in data['zerver_stream']:
+        if 'rendered_description' in stream:
+            continue
+        stream["rendered_description"] = bugdown_convert(stream["description"])
     bulk_import_model(data, Stream)
 
     realm.notifications_stream_id = notifications_stream_id

With those changes, merged, thanks @Hypro999! I think this should just leave the UI changes, which I imagine should be fairly simple everywhere except for the stream-edit UI.

@timabbott timabbott closed this Feb 2, 2019
@Hypro999 Hypro999 changed the title [WIP] Render Stream Descriptions From the Backend Render Stream Descriptions From the Backend Feb 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants