Introduction

This is essentially a big brain dump for those who will be continuing to work on the forums code since I will be moving to a new product area soon.

Dead Features

There are several features that were either never completed or were removed but still have bits of code to support them floating around in both the back end and front end. These bits should be removed, as they have suffered from bit rot and are unnecessary distractions.

• Tagging of posts (including a feature to find related posts by tag similarity)
• Highlighting text within posts (presumably in search results)

LMS Python code

Permissions

The django_comment_common app contains a framework for establishing roles with orthogonal sets of permissions. However, our current roles do not take advantage of this fact, and I am unaware of any feature request or other plan that would change that. The Permission model should be eliminated, and we should check whether a user has at least a particular privilege level instead of checking that the user has a role that has a specific named permission. Additionally, the permissions for all views are declared in a single dictionary in django_comment_client/permissions.py; it would be much better to declare the permissions for each view in a decorator on the view function.

Another nice thing to do would be to only allow each user to have one Role. That would be more difficult in two ways: 1) We would probably want to change the way roles are granted on the instructor dashboard, since they are currently presented as group membership, and 2) We would have to write an irreversible database migration that would remove all but the highest-privilege role from each user with multiple roles.

Finally, the Administrator role should be eliminated. The only permission that it currently has is "manage_moderators", but that permission is not referenced anywhere else in the code. Moderator management is actually done via the instructor dashboard, which is available to course and global staff regardless of forum role.

comment_client

It's trying to be an ODM, but that's more complexity than we need. I haven't given this one a ton of thought, but my guess is that the code would be much more readable if it were turned into a handful of single-purpose functions.

django_comment_client

annotated_content_info

Each of the views in django_comment_client.forum.views and some of the views in django_comment_client.base.views return a dictionary that maps content ids to supplementary information about that item (whether the user has voted on or subscribed to it and what abilities the user has with regard to it). In the front end, that mapping is then loaded into a global variable for reference when rendering. Instead, that kind of information should simply be included directly in the content.

prepare_content

This function in django_comment_client.utils was formerly called safe_content. It was originally responsible for sanitizing data but now also does some augmentation as well. Ultimately, it should handle all augmentation and sanitization, and its helpers should be renamed to be module private.

forum.views

We should use separate endpoints for HTML and AJAX requests rather than delivering radically different payloads depending on a header. Ideally, there would be a single view for loading the discussion app, a Backbone router in the front end would load the appropriate view within that based on the URL, and data would be sent to the client via AJAX.

base.views

It would be nice to make conform more to the pattern of our other REST APIs and be more resource oriented than action oriented.

Feature flags

ENABLE_DISCUSSION_SERVICE

If this setting is False, then we prevent access to forum endpoints by not including them in lms/urls.py. IMO, it would be better to guard the views themselves with a decorator that would raise Http404.

ENABLE_DISCUSSION_HOME_PANEL

This controls whether or not the discussion tab when initially loaded (without accessing a specific thread) contains the table with example icons, help text, and the notification setting checkbox. It should be replaced with an ENABLE_NOTIFIER setting that would control only the presence of the notification setting checkbox, and the help information should always appear.

Templates

Everything should be pure Underscore except for the main template for the discussion tab and module and (for now) the user profile, which exists as a completely separate page. The main templates should basically just contain an element into which the Backbone app can insert itself. We currently have some Mustache templates that should be straightforward to convert, and the _underscore_templates.html file contains many templates that should have all of the server-side (Mako) logic converted to front-end logic and be factored out into separate template files. The latter process has already begun.

On a more minor note, the "home panel" (which appears when the discussion tab is first loaded and contains help and the control for subscribing to notifications) is currently feature flagged. That feature flag should be removed, and a separate one should be added to control only whether the notification subscription control appears.

Front End (CoffeeScript/JavaScript)

General Stuff

CoffeeScript => JavaScript

Per the more general organizational direction, we should avoid adding new CoffeeScript code and gradually convert our CoffeeScript code to JavaScript.

require.js and file paths/names

We should use require.js for this code. One of the nasty consequences of not having done so is that the directory structure and file naming are crafted so that the files are loaded according to dependency order. This results in files that are not in the right place, oddly named classes and files, and multiple classes in one file. As we implement require.js, we should be sure to fix all of these problems.

Remove globals

Several bits of state are stored in global scope, including the list of users with privileged roles, the current user, and the course id. These should all be passed as appropriate to the objects and functions that need them.

Markdown and MathJax

These are consistent pain points for their complexity. In testing, we frequently turn functions related to these features into spies to avoid invoking them. We should figure out ways to isolate them and simplify the code that invokes them.

Use <button> instead of <a>

In many views and templates, we have special code to ensure that our things that look like buttons also act like buttons (i.e. use the button role and activate upon pressing the spacebar). We should really just use button elements, but we must ensure that we make any necessary CSS changes to avoid changing their appearance.

Better use of the Backbone framework

RESTful data transfer

Backbone has nice facilities for fetching and persisting data via REST APIs. Once the LMS APIs are rewritten to adhere more to the REST pattern, we can simplify the front end code by taking advantage of these.

Use events to communicate between views instead of shared state

Currently, we have several instances where the same model or collection is used by multiple views, and such views communicate by modifying the model or collection and/or setting callbacks. In some cases, this is desirable (e.g. any update to a Thread from any view will cause its rendering in the sidebar to update accordingly). In other cases, this can be confusing (e.g. NewPostView having a collection to which it adds new posts). We should think carefully about whether shared state or events are appropriate for each situation.

Use .listenTo instead of .on

http://stackoverflow.com/questions/16823746/backbone-js-listento-vs-on

Let views create their own elements

Most of the time, we pass an existing element in the DOM as the el option when constructing a view. It is more efficient and concise to define tagName and className on the view and add its element to the page when rendering has completed (because the browser can do a single update).

Jasmine tests

Many of our Jasmine tests are really doing behavioral testing as opposed to unit testing. We should try to find ways to make these classes more unit testable and do behavioral testing in bok choy, which is the more appropriate framework for that kind of thing.

Specific files (paths relative to common/static/coffee/src/discussion)

discussion_filter.coffee

Duplicates functionality in DiscussionThreadListView; the two should share code to the extent possible.

discussion_router.coffee

This class currently acts as a main entry point for the discussion tab app. We should separate that behavior into another class and make DiscussionRouter actually just a Backbone Router. Also, we should implement more routes than the two currently available, which are the discussion home and the single-thread view. At the very least, we should have a route for a specific topic.

views/discussion_thread_list_view.coffee

This class currently does way too much. It manages all of the elements in the sidebar itself. This functionality should be broken out into more specific views.

The goHome function is only invoked by DiscussionRouter. It was originally implemented on DiscussionThreadListView because the sidebar used to have a home button that would return to the discussion home screen, but that functionality should really have resided all along in whatever view contains DiscussionThreadListView.

The updateSidebar function ensures that the sidebar appears to stay fixed relative to the viewport (but within the confines of the header/footer). This has historically been error prone, and it is computationally relatively expensive for something that is executed every time the user scrolls. Along with the fact that some users have specifically expressed displeasure with the feature, we should consider simply removing it.

views/discussion_thread_profile_view.coffee

This view should be consolidated with the other thread views. It used to be the case that the user profile page acted similarly to an inline discussion (i.e. each thread was presented in an abbreviated form and could be expanded and viewed inline). I do not know why this functionality was removed, but given the recent refactoring of the thread views, it could probably be reinstated without much difficulty.

views/discussion_thread_view.coffee

I think this view should be broken into four parts:

1. A full thread view (i.e. most of what it does now)
2. An abbreviated thread view
3. A container view that would switch between the abbreviated and full views when the thread is expanded/collapsed. This view would only be used for inline discussions (and maybe user profiles at some point)
4. A new response view

This would avoid complexity around re-rendering the body content (which has caused bugs around failing to render markup and XSS attacks) and hiding specific pieces of the full view (look through the templates for the post-extended-content class).

views/thread_response_view.coffee

The new comment view should be split off from this view.