Re: [GMG-Devel] [GSoC 2016] Tasks remaining

[Christopher Allan Webber]

Okay so... this is a bit long.  Here's my code review.

The second bullet point could use some feedback from others, especially
breton/Boris.

 - Some commits as "root" and "Ubuntu"... please rebase the branch and
   fix the author field of these commits
 - Right now there's kind of an intermediate state between being a
   plugin and being "core" code.  First of all, it does have its own
   custom_subtitles plugin path, but:
   
   #+BEGIN_SRC diff
--- /dev/null
+++ 
b/mediagoblin/db/migrations/versions/afd3d1da5e29_subtitle_plugin_initial_migration.py
@@ -0,0 +1,36 @@
+"""Subtitle plugin initial migration
+
+Revision ID: afd3d1da5e29
+Revises: 228916769bd2
+Create Date: 2016-06-03 11:48:03.369079
+
+"""
+
+# revision identifiers, used by Alembic.
+revision = 'afd3d1da5e29'
+down_revision = '228916769bd2'
+branch_labels = ('subtitles_plugin',)
+depends_on = None
   +
   #+END_SRC
   
   This has its own unique branch label.  But if we look at the
   models:

   #+BEGIN_SRC diff
--- a/mediagoblin/db/models.py
+++ b/mediagoblin/db/models.py
@@ -573,6 +573,15 @@ class MediaEntry(Base, MediaEntryMixin, CommentingMixin):
             name=v["name"], filepath=v["filepath"])
         )
 
+    subtitle_files_helper = relationship("MediaSubtitleFile",
+        cascade="all, delete-orphan",
+        order_by="MediaSubtitleFile.created"
+        )
+    subtitle_files = association_proxy("subtitle_files_helper", "dict_view",
+        creator=lambda v: MediaSubtitleFile(
+            name=v["name"], filepath=v["filepath"])
+        )
+
   #+END_SRC

   This is attached to the core.  The models should also be in
   mediagoblin/plugins/custom_subtitles/models.py if this is
   to be its own plugin.

   This applies to video only.  If anything, it should be attached to
   the video plugin, but not the main media entry.
   
   I'm actually all for the subtitle plugin becoming a "core" part of
   the video plugin, if others are as well.  I think in general it's a
   nice feature.  Having a boolean (as currently sort of exists,
   though it should be moved to a different section) 

   So a decision needs to be made about whether or not the subtitle
   stuff gets added to the video code proper or is its own plugin,
   with a dependency on the video plugin.  It shouldn't be attached
   to core, at any case.

   I'd also be interested in breton weighing in on this one.
   
 - path_subtitle is duplicated twice.
 - path_subtitle is also only used once... is it worth the decorator?
 - '/c_s/<string:path>/edit/' for custom_subtitles is kind of a weird url
 - so edit_subtitles could maybe reduce a level of indentation by doing this:

#+BEGIN_SRC python
  def edit_subtitles(request, media):
      if not mg_globals.app_config['allow_subtitles']:
          raise Forbidden("Subtitles are disabled")
#+END_SRC

 - it also looks like the "if 'subtitle_file' in request.files" is really more
   about splitting things up between when there's a GET or a POST?
   Also, is there any error handling that should be done?
   Consider looking at how other views utilize wtforms to both handle
   errors on forms.
 - It does look like you're using the public store for subtitle files.
   Good!
 - subtitle, not subttitle, in the message flashed from edit_subtitles
 - Instead of doing the try: finally: on the subtitle_public_file, you
   can use a "with" statement, which will automatically close it once it
   leaves the with's scope.
 - Not critical, feel free to ask on irc for clarity: It might be
   possible to write the file in chunks without having to pass the
   .read() into the store file's .write()... I haven't tried this
   myself with this particular file, but the write method on our file
   interface will try to do a chunked write if it can if one file
   itself is passed to its write method.
 - It's a good idea to do a redirect after handling a POST.  This means
   that if a user does something, if they press the refresh button, it
   won't simply resubmit the same data.
 - If you add new files, please add the appropriate copyright headers!
 - I notice that custom_subtitles uses open_subtitle.  However, consider
   that your subtitle has, at this point, been stored in the public_store.
   You're constructing things from an absolute path,
 - As for the css, I'm worried that it adds something about the "body"
   element.  Why is this?
 - Also css wise, probably more unique names should be chosen than
   "box" (almost everything in css is "boxy") and "overlay".  Maybe
   prefixing with "lightbox-" is good enough?
 - In custom_subtitles, split this line up:
#+BEGIN_SRC html
     <form action="{{ 
request.urlgen('mediagoblin.edit.custom_subtitles',path=path) }}" method="POST" 
enctype="multipart/form-data">
#+END_SRC
   Like so:
#+BEGIN_SRC html
     <form action="{{ 
request.urlgen('mediagoblin.edit.custom_subtitles',path=path) }}"
           method="POST"
           enctype="multipart/form-data">
#+END_SRC
 - This line should be indented one space for consistency:
#+BEGIN_SRC html
     {{ csrf_token }}
#+END_SRC
 - Same in edit_subtitles.
 - The various .html files should go in the plugin's templates
   directory...  be that the video media type's templates or the
   subtitle plugin's directory.
 - Similarly, the assets should be in the plugin's own directory.
   Look up how the "assetlink" system works in the mediagoblin docs,
   and look at how other plugins provide their own assets.
 - If subtitles are their own plugin and not a video media type,
   then they should use a hook to insert into video.html,
   rather than the addition to the template as done now.
 - I'm not a big fan of inline javascript as in custom_subtitles.html...
   for one thing, it's very risky.  Consider the part that has this:
   
   #+BEGIN_SRC html
<script>
var xmlhttp, text;
xmlhttp = new XMLHttpRequest();
xmlhttp.open('GET', '{{ path }}', false);
xmlhttp.send();
text = xmlhttp.responseText;
document.getElementById('test').value = text;   
</script>
   #+END_SRC
   
   Consider: a malicious actor could be very clever and use something to 
   do code injection through the {{ path }} element (look up xkcd comic
   of "little bobby tables" to see what I mean...!)
   
   Instead, separate the javascript out into its own file.  To provide
   the "path" variable to the script, provide an html element like this:
   
     <input type="hidden" id="subtitles-get-path"
            value="{{ path }}" />
          
   Then in your javascript, use the DOM to extract this element's value.

 - *Don't* edit media.html.  Use the media_sideinfo hook!
   
 - Instead of adding code to the delete_media_files function,
   we should provide a hook in this function that you should make use of.
   Probably a "hook_runall" call.
   
 - tools/subtitles.py should be actually be a "tools.py" in the subtitles
   plugin's directory.
   
 - The method you're using to open_subtitle() and save_subtitle() here
   are the reasons your code isn't working with the cloud storage code.
   Instead, you should use the storage api's own methods for opening
   and saving files.  Don't access their file paths on disk directly!
   If you do this, your code will be generic enough to work with *all*
   public store systems we provide.
   
 - Likewise, rather than putting the routes in user_pages/routing.py,
   you should instead handle them in your plugin directly.  Take a
   look at how mediagoblin/plugins/basic_auth/__init__.py does this,
   in the setup_plugin section.

I know that's a lot!  But I also know that a lot of these things are
hard to pick up without someone walking you through them.

Feel free to ping me or reply to here if you have any questions!
 - Chris