Skip to content

Instantly share code, notes, and snippets.

@thedeveloperr

thedeveloperr/gsoc_2019_zulip_workproduct.md

Last active January 9, 2020 14:10
Show Gist options
  • Save thedeveloperr/4c2630545b4c052deb45a7d86077e75c to your computer and use it in GitHub Desktop.
Save thedeveloperr/4c2630545b4c052deb45a7d86077e75c to your computer and use it in GitHub Desktop.
Download ZIP
This Gist contains work done for Zulip Open Source Project's Team chat software during Google summer of code 2019
Raw
gsoc_2019_zulip_workproduct.md

Work Product Document for Zulip open source project as GSoC 2019 student developer.

Mohit Gupta | Github | Linkedin

Areas worked in: Search, Message View, group PMs, bots, production

Introduction

This gist contains work done during Google Summer of Code 2019 with Zulip open source project. Most of my work is fullstack. I Worked both on:

  • Zulip's main frontend webapp which is in Javascript
  • Zulip backend server which uses Python's Django, sqlalchemy, tornado, rabbitmq.

I want to wiht my whole heart thank my GSoC's mentors Yago González, David Rosa Your patience, advice and regular meetings helped me to complete my work successfully. I also want to thank Zulip's Tim Abbott, Rishi Gupta for their guidance and code reviews. Thanks to the whole Zulip community at chat.zulip.org for their help during my GSoC journey. It's because of you all I’ve had an awesome summer. This whole experience helped me to grow and improve as a Software Developer for which I am grateful.

Work summary

  • PR: streams:public and streams:subscribed to search entire history of organisation. #12999

    • Status: Closed and Merged as commit e5482ad
    • Solves Issue #8859.Right now in zulip past message history can be searched in single stream one at a time. But This PR provides user ability to search all of the organisation's public streams (both subscribed and unsubscribed) history at once, including messages from before the user joint the organisation.
    • In zulip there are three streams, public, private, private with shared history. Initially spec. was to only allow search in public streams. I completed the work along with tests. Initially the syntax to search was is:public-stream. After review Tim suggested to expand it to public and private subscribed stream too. So now syntax is:
      • streams:public fetches all the history of public streams
      • streams:subscribed fetches all the history of subscribed streams public and private streams (which allows shared history) both.

    Results for streams:public is same as is:public-stream The only difference is syntax. Work for streams:public is complete and merged. This feature I think completes a feature that is needed in a team chat software. Ability to search whole organisation message history will allow users to find information that have been shared already in past even if it is hidden in an unsubscibed stream within organisation.

    • Work for streams:subscribed remains and will be done in a followup PR, it's being tracked as issue #13052

  • PR: filter: Add more narrow filters that cannot mark messages as read. #12849

    • Status: Open and waiting for review.
    • Follow up based on comment on PR #12747. Adds more narrows where message is not marked as read.
    • As a product decision, the team decided to expand more search views/narrows where message should not be marked as read. Eg. while searching for messages which has link using has:link operator or search messages where you were mentioned is:mentioned using operator. Most of the time was spent in writing exhaustive tests so as to ensure everything is working. This feature had many possible test cases. Exhaustive testing will ensure that no issue will crop up now and in future.
    • Unmerged commits: https://github.com/zulip/zulip/pull/12849/commits

  • PR: narrow: Fix to show last message in narrow when narrow allows.#12847

    • Status: Open and waiting for review (The code paths are complicated and need in depth review).
    • Solves a feature that was supposed to properly work in last PR. In PR #12747 there was a situtation which required not to show unread message first but show last message. All logic and functions were implemented and called but UI was not updating. In initial manual testing the all other situations were tested except this. There was no automated test to check this new situation so this problem was gone unnoticed. This PR basically fixes it and adds test to ensure this kind of issue is caught in future. Lesson: Exhaustive automated tests ensures that feature functions properly, mistakes are caught early and feature won't break in future.
    • Unmerged commits: https://github.com/zulip/zulip/pull/12847/commits

  • PR: search: Don't mark messages as read in search narrow. #12747

    • Status: Merged and Closed.
    • Solves issue #12556. Basic thing it solved was to not mark new unread messages as read when they appear in search query for an older conversation.
    • This was a pretty high priority and highly requested feature by community, both Tim and Yago were excited to see it happen. The main work was finding relevant places to make changes in existing codebase. Zulip's frontend is old and big, some places the code is too complicated so it was a time consuming process to read the existing code and figure out where to make changes. Every new feature required new code files and lines to be read. The actual logic was simple. After some experience, writing unit tests was also not that difficult.
    • Merged commits: 6ec40cf, 648a60

  • PR: Fix searching and viewing Group PM and refactor of by_pm_with #12661

    • Status: Merged and Closed.
    • Solves issue #12601 and #12593
    • Intially #12593 seemed to be a backend bug but while Investigating bug lead to two completely different bugs. One was frontend bug (source was very hard to find) another as expected a backend one. Investigation details
    • The fix for bug and another refactor work #12601 overlapped so I solved both in a single PR using four commits. The refactor experience took me to one of the core data model of zulip the Recipient Model. I got a chance to understand how db schema and sqlalchemy db queries works. I also learnt how to write clean code while doing the refactor.
    • Merged Commits: 48786, c971576, 00cdba2, 45f87ff

  • PR: WIP Administrator Bot #12589

    • Status: Open and Work in progress due to some doubts in specs.
    • Solves issue #12424
    • A refactor prep. commits was merged db3d81613bf0530db
    • Ability to create bot with admin privileges by an admin is done (and checks to prevent non admin to do so). Also updated tests to check some side cases mentioned in comment. The only stuff that is missing is to add constraints what admin bot can't do but only humans can do. I first manually audited the whole codebase to find existing admin capabilities and human only capabilities ( see conversation ) Tim and Rishi discussed what to add and what not to. I started working on the given specs but later I switched to another work due to some doubts in specification of admin bots. The PR is stuck because I am still waiting for the doubts to be resolved.
    • Merged Commits: db3d816, Unmerged Commits: https://github.com/zulip/zulip/pull/12589/commits

  • PR: Cleaned and Rebased version of PR #10102 Multiple api keys #12511

    • Status: Open and Waiting for review
    • Solves merge conflicts, rebasing and cleaning of PR #10102
    • PR #10102 was started by my mentor, Yago during his GSoC 2018 work. I had to solve merge conflicts and rebase it with latest master. This PR basically allows user to have multiple api keys. As of time of writing, a Zulip user have a single regenratable API key. In original PR cache deletion of API key was not there but a cache deletion logic for single key had been added in codebase by Tim Abbott I extended that cache deletion of api key to work with multiple api key model ( See https://github.com/zulip/zulip/pull/12511/files#r291530222) In this process I learnt more about cache codebase of zulip. This PR improved my git skills and I learnt a lot about git's cherry picking, searching old commits, diffs.
    • In the second work period, merge conflicts again occurred with my PR 12511. I had to rebase and fix new set of failing tests. Thanks to a fellow GSoC student Hemanth for helping me fix these undocumented tests. This experience gave me insights how a fast moving project can easily break previous unmerged work.
    • Unmerged Commits: https://github.com/zulip/zulip/pull/12511/commits

  • PR: bots: Bots can post to announcement-only streams if their owner can. #12383

    • Status: Merged and Closed
    • Solves issue #12310
    • Most of time was spent writing and correcting Unit test because of a buggy test endpoint. Intially wrote test using API endpoint that was used by existing tests already in the main codebase. But those pre existing testing code were buggy. I fixed those tests in commit d60f6c9. Later wrote correct tests for issue #12383. This experience taught me valid pre existing codebase or tests can have bugs in them. Always lookout for code around you and investigate any weird exiting code even if it's not directly related to your current work. Try to follow the principle of "Leave the code better than you found it"
    • Merged Commits: d60f6c9, a98447b

  • PR: Batch read flag requests to at most 1k message IDs #12264

    • Status: Merged and Closed
    • Solves issue #11956 of production server holding hundreds of locks at the same time. Frontend UIs may attempt to mark 10,000s of messages and query for flagging message in backend could run for extremely long periods of time (e.g. 72478s in one instance!).
    • Solution implemented is to make webapp batch the message flagging requests to at most ~1K message IDs at a time. Most of the work went into writing frontend unit tests to ensure that feature is working correctly. I figured out a way to intercept the request in javascript and ensured that only list of id of length equal to the batch size is being sent as request payload. I mocked server response and mark messages in UI and ensure already flagged message ids are not being sent in the next batch. This PR gave me experience to simulate different conditions in tests.
    • Merged Commits: 1902f52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment