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.
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.
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