From 180bdbde93ad4b62395c78e99e97587b44ad31c7 Mon Sep 17 00:00:00 2001 From: Elrond Date: Wed, 8 Jun 2011 23:22:11 +0200 Subject: [PATCH 01/30] Refactor filename generation in the public store Just a small refactoring of the filename setup in the public store. Very simple. --- mediagoblin/process_media/__init__.py | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/mediagoblin/process_media/__init__.py b/mediagoblin/process_media/__init__.py index 4f06a686..097b4375 100644 --- a/mediagoblin/process_media/__init__.py +++ b/mediagoblin/process_media/__init__.py @@ -24,6 +24,13 @@ from mediagoblin.globals import database, queue_store, public_store THUMB_SIZE = 200, 200 +def create_pub_filepath(entry, filename): + return public_store.get_unique_filepath( + ['media_entries', + unicode(entry['_id']), + filename]) + + @task def process_media_initial(media_id): entry = database.MediaEntry.one( @@ -36,10 +43,7 @@ def process_media_initial(media_id): thumb = Image.open(queued_file) thumb.thumbnail(THUMB_SIZE, Image.ANTIALIAS) - thumb_filepath = public_store.get_unique_filepath( - ['media_entries', - unicode(entry['_id']), - 'thumbnail.jpg']) + thumb_filepath = create_pub_filepath(entry, 'thumbnail.jpg') with public_store.get_file(thumb_filepath, 'w') as thumb_file: thumb.save(thumb_file, "JPEG") @@ -49,15 +53,13 @@ def process_media_initial(media_id): queued_file = queue_store.get_file(queued_filepath, 'rb') with queued_file: - main_filepath = public_store.get_unique_filepath( - ['media_entries', - unicode(entry['_id']), - queued_filepath[-1]]) + main_filepath = create_pub_filepath(entry, queued_filepath[-1]) with public_store.get_file(main_filepath, 'wb') as main_file: main_file.write(queued_file.read()) queue_store.delete_file(queued_filepath) + entry['queued_media_file'] = [] media_files_dict = entry.setdefault('media_files', {}) media_files_dict['thumb'] = thumb_filepath media_files_dict['main'] = main_filepath From e893094a06ab5d659b82c2d2fe646b1545c131dd Mon Sep 17 00:00:00 2001 From: Elrond Date: Fri, 10 Jun 2011 21:20:18 +0200 Subject: [PATCH 02/30] celery_setup: drop param to setup_self and simplify OUR_MODULENAME setup_self used to look like this: setup_self(setup_globals_func=setup_globals) The function isn't called with any param, so drop it. Rewrite function as needed. The module var OUR_MODULENAME just has the module's name in it. This is available as __name__ anyway, so use this to initialize the var. --- mediagoblin/celery_setup/from_celery.py | 7 +++---- mediagoblin/celery_setup/from_tests.py | 5 ++--- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/mediagoblin/celery_setup/from_celery.py b/mediagoblin/celery_setup/from_celery.py index 0669e80c..6fab59ca 100644 --- a/mediagoblin/celery_setup/from_celery.py +++ b/mediagoblin/celery_setup/from_celery.py @@ -23,13 +23,12 @@ from mediagoblin import storage from mediagoblin.db.open import setup_connection_and_db_from_config from mediagoblin.celery_setup import setup_celery_from_config from mediagoblin.globals import setup_globals -from mediagoblin import globals as mgoblin_globals -OUR_MODULENAME = 'mediagoblin.celery_setup.from_celery' +OUR_MODULENAME = __name__ -def setup_self(setup_globals_func=setup_globals): +def setup_self(): """ Transform this module into a celery config module by reading the mediagoblin config file. Set the environment variable @@ -76,7 +75,7 @@ def setup_self(setup_globals_func=setup_globals): queue_store = storage.storage_system_from_paste_config( mgoblin_section, 'queuestore') - setup_globals_func( + setup_globals( db_connection=connection, database=db, public_store=public_store, diff --git a/mediagoblin/celery_setup/from_tests.py b/mediagoblin/celery_setup/from_tests.py index fe7d7314..70814075 100644 --- a/mediagoblin/celery_setup/from_tests.py +++ b/mediagoblin/celery_setup/from_tests.py @@ -19,13 +19,12 @@ import os from mediagoblin.tests.tools import TEST_APP_CONFIG from mediagoblin import util from mediagoblin.celery_setup import setup_celery_from_config -from mediagoblin.globals import setup_globals -OUR_MODULENAME = 'mediagoblin.celery_setup.from_tests' +OUR_MODULENAME = __name__ -def setup_self(setup_globals_func=setup_globals): +def setup_self(): """ Set up celery for testing's sake, which just needs to set up celery and celery only. From 12c559447a75fa4b43872ccda544762a331a1bed Mon Sep 17 00:00:00 2001 From: Elrond Date: Fri, 10 Jun 2011 21:59:04 +0200 Subject: [PATCH 03/30] Tests: Kill the whole testing database after all tests nose allows setup and teardown functions at the package level. So use this to drop the complete database after all tests have finished. --- mediagoblin/tests/__init__.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/mediagoblin/tests/__init__.py b/mediagoblin/tests/__init__.py index c129cbf8..46c7fd69 100644 --- a/mediagoblin/tests/__init__.py +++ b/mediagoblin/tests/__init__.py @@ -13,3 +13,14 @@ # # You should have received a copy of the GNU Affero General Public License # along with this program. If not, see . + +from .. import globals + + +def setup_package(): + pass + +def teardown_package(): + print "Killing db ..." + globals.db_connection.drop_database(globals.database.name) + print "... done" From 3a89c23e7fd330ea662dd57ff74a9d424d476b84 Mon Sep 17 00:00:00 2001 From: Christopher Allan Webber Date: Sat, 11 Jun 2011 11:18:03 -0500 Subject: [PATCH 04/30] Allow storage systems to be local and allow for a get_local_path method if applicable. --- mediagoblin/storage.py | 21 ++++++++++++++++++++- mediagoblin/tests/test_storage.py | 17 +++++++++++++++++ 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/mediagoblin/storage.py b/mediagoblin/storage.py index 5d7e70d6..685039b2 100644 --- a/mediagoblin/storage.py +++ b/mediagoblin/storage.py @@ -60,6 +60,9 @@ class StorageInterface(object): StorageInterface. """ + # Whether this file store is on the local filesystem. + local_storage = False + def __raise_not_implemented(self): """ Raise a warning about some component not implemented by a @@ -127,12 +130,25 @@ class StorageInterface(object): else: return filepath + def get_local_path(self, filepath): + """ + If this is a local_storage implementation, give us a link to + the local filesystem reference to this file. + + >>> storage_handler.get_local_path(['foo', 'bar', 'baz.jpg']) + u'/path/to/mounting/foo/bar/baz.jpg' + """ + # Subclasses should override this method, if applicable. + self.__raise_not_implemented() + class BasicFileStorage(StorageInterface): """ Basic local filesystem implementation of storage API """ + local_storage = True + def __init__(self, base_dir, base_url=None, **kwargs): """ Keyword arguments: @@ -177,6 +193,9 @@ class BasicFileStorage(StorageInterface): self.base_url, '/'.join(clean_listy_filepath(filepath))) + def get_local_path(self, filepath): + return self._resolve_filepath(filepath) + ########### # Utilities @@ -187,7 +206,7 @@ def clean_listy_filepath(listy_filepath): Take a listy filepath (like ['dir1', 'dir2', 'filename.jpg']) and clean out any nastiness from it. - For example: + >>> clean_listy_filepath([u'/dir1/', u'foo/../nasty', u'linooks.jpg']) [u'dir1', u'foo_.._nasty', u'linooks.jpg'] diff --git a/mediagoblin/tests/test_storage.py b/mediagoblin/tests/test_storage.py index 61dd5dca..5efed405 100644 --- a/mediagoblin/tests/test_storage.py +++ b/mediagoblin/tests/test_storage.py @@ -214,3 +214,20 @@ def test_basic_storage_url_for_file(): ['dir1', 'dir2', 'filename.txt']) expected = 'http://media.example.org/ourmedia/dir1/dir2/filename.txt' assert result == expected + + +def test_basic_storage_get_local_path(): + tmpdir, this_storage = get_tmp_filestorage() + + result = this_storage.get_local_path( + ['dir1', 'dir2', 'filename.txt']) + + expected = os.path.join( + tmpdir, 'dir1/dir2/filename.txt') + + assert result == expected + + +def test_basic_storage_is_local(): + tmpdir, this_storage = get_tmp_filestorage() + assert this_storage.local_storage is True From 6a07362dd0145643edf4b61b0ae402e3093793c2 Mon Sep 17 00:00:00 2001 From: Christopher Allan Webber Date: Sat, 11 Jun 2011 12:04:30 -0500 Subject: [PATCH 05/30] Adding a copy_locally() method to the StorageInterface and giving it a test. --- mediagoblin/storage.py | 21 +++++++++++++++++++++ mediagoblin/tests/test_storage.py | 16 ++++++++++++++++ 2 files changed, 37 insertions(+) diff --git a/mediagoblin/storage.py b/mediagoblin/storage.py index 685039b2..ba6ac017 100644 --- a/mediagoblin/storage.py +++ b/mediagoblin/storage.py @@ -16,6 +16,7 @@ import os import re +import shutil import urlparse import uuid @@ -141,6 +142,24 @@ class StorageInterface(object): # Subclasses should override this method, if applicable. self.__raise_not_implemented() + def copy_locally(self, filepath, dest_path): + """ + Copy this file locally. + + A basic working method for this is provided that should + function both for local_storage systems and remote storge + systems, but if more efficient systems for copying locally + apply to your system, override this method with something more + appropriate. + """ + if self.local_storage: + shutil.copy( + self.get_local_path(filepath), dest_path) + else: + with self.get_file(filepath, 'rb') as source_file: + with file(dest_path, 'wb') as dest_file: + dest_file.write(source_file.read()) + class BasicFileStorage(StorageInterface): """ @@ -272,3 +291,5 @@ def storage_system_from_paste_config(paste_config, storage_prefix): storage_class = util.import_component(storage_class) return storage_class(**config_params) + + diff --git a/mediagoblin/tests/test_storage.py b/mediagoblin/tests/test_storage.py index 5efed405..83f893f1 100644 --- a/mediagoblin/tests/test_storage.py +++ b/mediagoblin/tests/test_storage.py @@ -231,3 +231,19 @@ def test_basic_storage_get_local_path(): def test_basic_storage_is_local(): tmpdir, this_storage = get_tmp_filestorage() assert this_storage.local_storage is True + + +def test_basic_storage_copy_locally(): + tmpdir, this_storage = get_tmp_filestorage() + + dest_tmpdir = tempfile.mkdtemp() + + filepath = ['dir1', 'dir2', 'ourfile.txt'] + with this_storage.get_file(filepath, 'w') as our_file: + our_file.write('Testing this file') + + new_file_dest = os.path.join(dest_tmpdir, 'file2.txt') + + this_storage.copy_locally(filepath, new_file_dest) + + assert file(new_file_dest).read() == 'Testing this file' From 68784b9e6ad98cef88d5e9508e9536dc296b09e2 Mon Sep 17 00:00:00 2001 From: Christopher Allan Webber Date: Sat, 11 Jun 2011 16:48:39 -0500 Subject: [PATCH 06/30] Base structure of workbench manager --- mediagoblin/process_media/workbench.py | 73 ++++++++++++++++++++++++++ 1 file changed, 73 insertions(+) create mode 100644 mediagoblin/process_media/workbench.py diff --git a/mediagoblin/process_media/workbench.py b/mediagoblin/process_media/workbench.py new file mode 100644 index 00000000..41ccc081 --- /dev/null +++ b/mediagoblin/process_media/workbench.py @@ -0,0 +1,73 @@ +# GNU MediaGoblin -- federated, autonomous media hosting +# Copyright (C) 2011 Free Software Foundation, Inc +# +# This program is free software: you can redistribute it and/or modify +# it under the terms of the GNU Affero General Public License as published by +# the Free Software Foundation, either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU Affero General Public License for more details. +# +# You should have received a copy of the GNU Affero General Public License +# along with this program. If not, see . + +import tempfile + + +class WorkbenchOutsideScope(Exception): + """ + Raised when a workbench is outside a WorkbenchManager scope. + """ + pass + + +# TODO: This doesn't seem like it'd work with Windows +DEFAULT_WORKBENCH_DIR = u'/tmp/workbench/' + + +class WorkbenchManager(object): + """ + A system for generating and destroying workbenches. + + Workbenches are actually just subdirectories of a temporary storage space + for during the processing stage. + """ + + def __init__(self, base_workbench_dir): + self.base_workbench_dir = base_workbench_dir + + def create_workbench(self): + """ + Create and return the path to a new workbench (directory). + """ + pass + + def destroy_workbench(self, workbench): + """ + Destroy this workbench! Deletes the directory and all its contents! + + Makes sure the workbench actually belongs to this manager though. + """ + pass + + def possibly_localize_file(self, workbench, storage, filepath): + """ + Possibly localize the file from this storage system (for read-only + purposes, modifications should be written to a new file.). + + If the file is already local, just return the absolute filename of that + local file. Otherwise, copy the file locally to the workbench, and + return the absolute path of the new file. + + Also returns whether or not it copied the file locally. + + Returns: + (localized_filename, copied_locally) + The first of these bieng the absolute filename as described above as a + unicode string, the second being a boolean stating whether or not we + had to copy the file locally. + """ + pass From 678d1a20ea0fa674253b835ca2c07b595fd8728c Mon Sep 17 00:00:00 2001 From: Christopher Allan Webber Date: Sat, 11 Jun 2011 17:28:58 -0500 Subject: [PATCH 07/30] Wrote functions and documentation for all the WorkbenchManager functions but haven't actually tested them yet. :) --- mediagoblin/process_media/workbench.py | 83 +++++++++++++++++++++++--- 1 file changed, 75 insertions(+), 8 deletions(-) diff --git a/mediagoblin/process_media/workbench.py b/mediagoblin/process_media/workbench.py index 41ccc081..32ebe534 100644 --- a/mediagoblin/process_media/workbench.py +++ b/mediagoblin/process_media/workbench.py @@ -14,9 +14,18 @@ # You should have received a copy of the GNU Affero General Public License # along with this program. If not, see . +import os +import shutil import tempfile +DEFAULT_WORKBENCH_DIR = os.path.join( + tempfile.gettempdir(), u'mgoblin_workbench') + + +# Exception(s) +# ------------ + class WorkbenchOutsideScope(Exception): """ Raised when a workbench is outside a WorkbenchManager scope. @@ -24,9 +33,8 @@ class WorkbenchOutsideScope(Exception): pass -# TODO: This doesn't seem like it'd work with Windows -DEFAULT_WORKBENCH_DIR = u'/tmp/workbench/' - +# Actual workbench stuff +# ---------------------- class WorkbenchManager(object): """ @@ -37,13 +45,15 @@ class WorkbenchManager(object): """ def __init__(self, base_workbench_dir): - self.base_workbench_dir = base_workbench_dir + self.base_workbench_dir = os.path.abspath(base_workbench_dir) + if not os.path.exists(self.base_workbench_dir): + os.makedirs(self.base_workbench_dir) def create_workbench(self): """ Create and return the path to a new workbench (directory). """ - pass + return tempfile.mkdtemp(dir=self.base_workbench_dir) def destroy_workbench(self, workbench): """ @@ -51,9 +61,18 @@ class WorkbenchManager(object): Makes sure the workbench actually belongs to this manager though. """ - pass + # just in case + workbench = os.path.abspath(workbench) - def possibly_localize_file(self, workbench, storage, filepath): + if not workbench.startswith(self.base_workbench_dir): + raise WorkbenchOutsideScope( + "Can't destroy workbench outside the base workbench dir") + + shutil.rmtree(workbench) + + def possibly_localize_file(self, workbench, storage, filepath, + filename_if_copying=None, + keep_extension_if_copying=True): """ Possibly localize the file from this storage system (for read-only purposes, modifications should be written to a new file.). @@ -62,6 +81,14 @@ class WorkbenchManager(object): local file. Otherwise, copy the file locally to the workbench, and return the absolute path of the new file. + If it is copying locally, we might want to require a filename like + "source.jpg" to ensure that we won't conflict with other filenames in + our workbench... if that's the case, make sure filename_if_copying is + set to something like 'source.jpg'. Relatedly, if you set + keep_extension_if_copying, you don't have to set an extension on + filename_if_copying yourself, it'll be set for you (assuming such an + extension can be extacted from the filename in the filepath). + Also returns whether or not it copied the file locally. Returns: @@ -69,5 +96,45 @@ class WorkbenchManager(object): The first of these bieng the absolute filename as described above as a unicode string, the second being a boolean stating whether or not we had to copy the file locally. + + Examples: + >>> wb_manager.possibly_localize_file( + ... '/our/workbench/subdir', local_storage, + ... ['path', 'to', 'foobar.jpg']) + (u'/local/storage/path/to/foobar.jpg', False) + + >>> wb_manager.possibly_localize_file( + ... '/our/workbench/subdir', remote_storage, + ... ['path', 'to', 'foobar.jpg']) + (u'/our/workbench/subdir/foobar.jpg', True) + + >>> wb_manager.possibly_localize_file( + ... '/our/workbench/subdir', remote_storage, + ... ['path', 'to', 'foobar.jpg'], 'source.jpeg', False) + (u'/our/workbench/subdir/foobar.jpeg', True) + + >>> wb_manager.possibly_localize_file( + ... '/our/workbench/subdir', remote_storage, + ... ['path', 'to', 'foobar.jpg'], 'source', True) + (u'/our/workbench/subdir/foobar.jpg', True) """ - pass + if storage.local_storage: + return (storage.get_local_path(filepath), False) + else: + if filename_if_copying is None: + dest_filename = filepath[-1] + else: + orig_filename, orig_ext = os.path.splitext(filepath[-1]) + if keep_extension_if_copying and orig_ext: + dest_filename = filename_if_copying + '.' + orig_ext + else: + dest_filename = filename_if_copying + + full_dest_filename = os.path.join( + workbench, dest_filename) + + # copy it over + storage.copy_locally( + filepath, full_dest_filename) + + return (full_dest_filename, True) From 2616d70903a775ee3790bdd38f6f0ad4003c0170 Mon Sep 17 00:00:00 2001 From: Christopher Allan Webber Date: Sat, 11 Jun 2011 18:49:04 -0500 Subject: [PATCH 08/30] Tests for creating/destroying workbenches --- mediagoblin/tests/test_workbench.py | 44 +++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) create mode 100644 mediagoblin/tests/test_workbench.py diff --git a/mediagoblin/tests/test_workbench.py b/mediagoblin/tests/test_workbench.py new file mode 100644 index 00000000..1d006645 --- /dev/null +++ b/mediagoblin/tests/test_workbench.py @@ -0,0 +1,44 @@ +# GNU MediaGoblin -- federated, autonomous media hosting +# Copyright (C) 2011 Free Software Foundation, Inc +# +# This program is free software: you can redistribute it and/or modify +# it under the terms of the GNU Affero General Public License as published by +# the Free Software Foundation, either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU Affero General Public License for more details. +# +# You should have received a copy of the GNU Affero General Public License +# along with this program. If not, see . + +import os +import tempfile + +from mediagoblin.process_media import workbench + + +class TestWorkbench(object): + def setUp(self): + self.workbench_manager = workbench.WorkbenchManager( + os.path.join(tempfile.gettempdir(), u'mgoblin_workbench_testing')) + + def test_create_workbench(self): + workbench = self.workbench_manager.create_workbench() + assert os.path.isdir(workbench) + assert workbench.startswith(self.workbench_manager.base_workbench_dir) + + def test_destroy_workbench(self): + # kill a workbench + workbench = self.workbench_manager.create_workbench() + tmpfile = file(os.path.join(workbench, 'temp.txt'), 'w') + with tmpfile: + tmpfile.write('lollerskates') + + assert os.path.exists(os.path.join(workbench, 'temp.txt')) + + self.workbench_manager.destroy_workbench(workbench) + assert not os.path.exists(os.path.join(workbench, 'temp.txt')) + assert not os.path.exists(workbench) From 2ecee34f08ad367f54f7a066a2e3ce81aba01f0f Mon Sep 17 00:00:00 2001 From: Christopher Allan Webber Date: Sat, 11 Jun 2011 18:52:48 -0500 Subject: [PATCH 09/30] Make sure workbench won't kill directories out of scope. --- mediagoblin/tests/test_workbench.py | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/mediagoblin/tests/test_workbench.py b/mediagoblin/tests/test_workbench.py index 1d006645..83e90c3d 100644 --- a/mediagoblin/tests/test_workbench.py +++ b/mediagoblin/tests/test_workbench.py @@ -17,6 +17,8 @@ import os import tempfile +from nose.tools import assert_raises + from mediagoblin.process_media import workbench @@ -32,13 +34,21 @@ class TestWorkbench(object): def test_destroy_workbench(self): # kill a workbench - workbench = self.workbench_manager.create_workbench() - tmpfile = file(os.path.join(workbench, 'temp.txt'), 'w') + this_workbench = self.workbench_manager.create_workbench() + tmpfile = file(os.path.join(this_workbench, 'temp.txt'), 'w') with tmpfile: tmpfile.write('lollerskates') - assert os.path.exists(os.path.join(workbench, 'temp.txt')) + assert os.path.exists(os.path.join(this_workbench, 'temp.txt')) - self.workbench_manager.destroy_workbench(workbench) - assert not os.path.exists(os.path.join(workbench, 'temp.txt')) - assert not os.path.exists(workbench) + self.workbench_manager.destroy_workbench(this_workbench) + assert not os.path.exists(os.path.join(this_workbench, 'temp.txt')) + assert not os.path.exists(this_workbench) + + # make sure we can't kill other stuff though + dont_kill_this = tempfile.mkdtemp() + + assert_raises( + workbench.WorkbenchOutsideScope, + self.workbench_manager.destroy_workbench, + dont_kill_this) From d91b5a7c2d8f518a0654d066c41b1359bee1d04e Mon Sep 17 00:00:00 2001 From: Christopher Allan Webber Date: Sat, 11 Jun 2011 19:17:44 -0500 Subject: [PATCH 10/30] Added a FakeRemoteStorage, for testing purposes --- mediagoblin/tests/test_storage.py | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/mediagoblin/tests/test_storage.py b/mediagoblin/tests/test_storage.py index 83f893f1..55b66e84 100644 --- a/mediagoblin/tests/test_storage.py +++ b/mediagoblin/tests/test_storage.py @@ -52,6 +52,11 @@ class FakeStorageSystem(): self.foobie = foobie self.blech = blech +class FakeRemoteStorage(storage.BasicFileStorage): + # Theoretically despite this, all the methods should work but it + # should force copying to the workbench + local_storage = False + def test_storage_system_from_paste_config(): this_storage = storage.storage_system_from_paste_config( @@ -81,9 +86,12 @@ def test_storage_system_from_paste_config(): # Basic file storage tests ########################## -def get_tmp_filestorage(mount_url=None): +def get_tmp_filestorage(mount_url=None, fake_remote=False): tmpdir = tempfile.mkdtemp() - this_storage = storage.BasicFileStorage(tmpdir, mount_url) + if fake_remote: + this_storage = FakeRemoteStorage(tmpdir, mount_url) + else: + this_storage = storage.BasicFileStorage(tmpdir, mount_url) return tmpdir, this_storage From f2b96ff0a4efab989ff5558de44b33da21b1bf00 Mon Sep 17 00:00:00 2001 From: Christopher Allan Webber Date: Sat, 11 Jun 2011 19:18:27 -0500 Subject: [PATCH 11/30] We don't need this extra '.' in making the filename --- mediagoblin/process_media/workbench.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mediagoblin/process_media/workbench.py b/mediagoblin/process_media/workbench.py index 32ebe534..b1198adf 100644 --- a/mediagoblin/process_media/workbench.py +++ b/mediagoblin/process_media/workbench.py @@ -126,7 +126,7 @@ class WorkbenchManager(object): else: orig_filename, orig_ext = os.path.splitext(filepath[-1]) if keep_extension_if_copying and orig_ext: - dest_filename = filename_if_copying + '.' + orig_ext + dest_filename = filename_if_copying + orig_ext else: dest_filename = filename_if_copying From f43ecb0fc459c0ee64f3a40b98ed6bbf8564c107 Mon Sep 17 00:00:00 2001 From: Christopher Allan Webber Date: Sat, 11 Jun 2011 19:18:51 -0500 Subject: [PATCH 12/30] test WorkbenchManager.possibly_localize_file() --- mediagoblin/tests/test_workbench.py | 43 +++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/mediagoblin/tests/test_workbench.py b/mediagoblin/tests/test_workbench.py index 83e90c3d..fd71a6eb 100644 --- a/mediagoblin/tests/test_workbench.py +++ b/mediagoblin/tests/test_workbench.py @@ -20,6 +20,7 @@ import tempfile from nose.tools import assert_raises from mediagoblin.process_media import workbench +from mediagoblin.tests.test_storage import get_tmp_filestorage class TestWorkbench(object): @@ -52,3 +53,45 @@ class TestWorkbench(object): workbench.WorkbenchOutsideScope, self.workbench_manager.destroy_workbench, dont_kill_this) + + def test_possibly_localize_file(self): + tmpdir, this_storage = get_tmp_filestorage() + this_workbench = self.workbench_manager.create_workbench() + + # Write a brand new file + filepath = ['dir1', 'dir2', 'ourfile.txt'] + + with this_storage.get_file(filepath, 'w') as our_file: + our_file.write('Our file') + + # with a local file storage + filename, copied = self.workbench_manager.possibly_localize_file( + this_workbench, this_storage, filepath) + assert copied is False + assert filename == os.path.join( + tmpdir, 'dir1/dir2/ourfile.txt') + + # with a fake remote file storage + tmpdir, this_storage = get_tmp_filestorage(fake_remote=True) + + # ... write a brand new file, again ;) + with this_storage.get_file(filepath, 'w') as our_file: + our_file.write('Our file') + + filename, copied = self.workbench_manager.possibly_localize_file( + this_workbench, this_storage, filepath) + assert filename == os.path.join( + this_workbench, 'ourfile.txt') + + # fake remote file storage, filename_if_copying set + filename, copied = self.workbench_manager.possibly_localize_file( + this_workbench, this_storage, filepath, 'thisfile') + assert filename == os.path.join( + this_workbench, 'thisfile.txt') + + # fake remote file storage, filename_if_copying set, + # keep_extension_if_copying set to false + filename, copied = self.workbench_manager.possibly_localize_file( + this_workbench, this_storage, filepath, 'thisfile.text', False) + assert filename == os.path.join( + this_workbench, 'thisfile.text') From 7ecc58cc5cf49c87001c38ba5d0607644ca195d4 Mon Sep 17 00:00:00 2001 From: Christopher Allan Webber Date: Sat, 11 Jun 2011 19:47:02 -0500 Subject: [PATCH 13/30] Have the application set up instances of the WorkbenchManager. --- mediagoblin/app.py | 11 ++++++++--- mediagoblin/celery_setup/from_celery.py | 10 ++++++++-- 2 files changed, 16 insertions(+), 5 deletions(-) diff --git a/mediagoblin/app.py b/mediagoblin/app.py index e5949531..da2b7d35 100644 --- a/mediagoblin/app.py +++ b/mediagoblin/app.py @@ -25,6 +25,8 @@ from mediagoblin import routing, util, storage, staticdirect from mediagoblin.db.open import setup_connection_and_db_from_config from mediagoblin.globals import setup_globals from mediagoblin.celery_setup import setup_celery_from_config +from mediagoblin.process_media.workbench import ( + WorkbenchManager, DEFAULT_WORKBENCH_DIR) class Error(Exception): pass @@ -39,7 +41,8 @@ class MediaGoblinApp(object): public_store, queue_store, staticdirector, email_sender_address, email_debug_mode, - user_template_path=None): + user_template_path=None, + workbench_path=DEFAULT_WORKBENCH_DIR): # Get the template environment self.template_loader = util.get_jinja_loader(user_template_path) @@ -66,7 +69,8 @@ class MediaGoblinApp(object): db_connection=connection, database=self.db, public_store=self.public_store, - queue_store=self.queue_store) + queue_store=self.queue_store, + workbench_manager=WorkbenchManager(workbench_path)) def __call__(self, environ, start_response): request = Request(environ) @@ -154,6 +158,7 @@ def paste_app_factory(global_config, **app_config): email_sender_address=app_config.get( 'email_sender_address', 'notice@mediagoblin.example.org'), email_debug_mode=asbool(app_config.get('email_debug_mode')), - user_template_path=app_config.get('local_templates')) + user_template_path=app_config.get('local_templates'), + workbench_path=app_config.get('workbench_path', DEFAULT_WORKBENCH_DIR)) return mgoblin_app diff --git a/mediagoblin/celery_setup/from_celery.py b/mediagoblin/celery_setup/from_celery.py index 0669e80c..e1216815 100644 --- a/mediagoblin/celery_setup/from_celery.py +++ b/mediagoblin/celery_setup/from_celery.py @@ -23,7 +23,8 @@ from mediagoblin import storage from mediagoblin.db.open import setup_connection_and_db_from_config from mediagoblin.celery_setup import setup_celery_from_config from mediagoblin.globals import setup_globals -from mediagoblin import globals as mgoblin_globals +from mediagoblin.process_media.workbench import ( + WorkbenchManager, DEFAULT_WORKBENCH_DIR) OUR_MODULENAME = 'mediagoblin.celery_setup.from_celery' @@ -76,6 +77,10 @@ def setup_self(setup_globals_func=setup_globals): queue_store = storage.storage_system_from_paste_config( mgoblin_section, 'queuestore') + workbench_manager = WorkbenchManager( + mgoblin_section.get( + 'workbench_path', DEFAULT_WORKBENCH_DIR)) + setup_globals_func( db_connection=connection, database=db, @@ -84,7 +89,8 @@ def setup_self(setup_globals_func=setup_globals): email_sender_address=mgoblin_section.get( 'email_sender_address', 'notice@mediagoblin.example.org'), - queue_store=queue_store) + queue_store=queue_store, + workbench_manager=workbench_manager) if os.environ['CELERY_CONFIG_MODULE'] == OUR_MODULENAME: From 894facc68d5a52dd796451361cd545f0bd3116f5 Mon Sep 17 00:00:00 2001 From: Christopher Allan Webber Date: Sat, 11 Jun 2011 19:48:49 -0500 Subject: [PATCH 14/30] Import mediagoblin.globals as mg_globals so we can be sure things are set up in the right order. --- mediagoblin/process_media/__init__.py | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/mediagoblin/process_media/__init__.py b/mediagoblin/process_media/__init__.py index 4f06a686..527c198c 100644 --- a/mediagoblin/process_media/__init__.py +++ b/mediagoblin/process_media/__init__.py @@ -18,7 +18,7 @@ import Image from mediagoblin.db.util import ObjectId from celery.task import task -from mediagoblin.globals import database, queue_store, public_store +from mediagoblin import globals as mg_globals THUMB_SIZE = 200, 200 @@ -26,38 +26,39 @@ THUMB_SIZE = 200, 200 @task def process_media_initial(media_id): - entry = database.MediaEntry.one( + entry = mg_globals.database.MediaEntry.one( {'_id': ObjectId(media_id)}) queued_filepath = entry['queued_media_file'] - queued_file = queue_store.get_file(queued_filepath, 'r') + queued_file = mg_globals.queue_store.get_file(queued_filepath, 'r') with queued_file: thumb = Image.open(queued_file) thumb.thumbnail(THUMB_SIZE, Image.ANTIALIAS) - thumb_filepath = public_store.get_unique_filepath( + thumb_filepath = mg_globals.public_store.get_unique_filepath( ['media_entries', unicode(entry['_id']), 'thumbnail.jpg']) - with public_store.get_file(thumb_filepath, 'w') as thumb_file: + thumb_file = mg_globals.public_store.get_file(thumb_filepath, 'w') + with thumb_file: thumb.save(thumb_file, "JPEG") # we have to re-read because unlike PIL, not everything reads # things in string representation :) - queued_file = queue_store.get_file(queued_filepath, 'rb') + queued_file = mg_globals.queue_store.get_file(queued_filepath, 'rb') with queued_file: - main_filepath = public_store.get_unique_filepath( + main_filepath = mg_globals.public_store.get_unique_filepath( ['media_entries', unicode(entry['_id']), queued_filepath[-1]]) - with public_store.get_file(main_filepath, 'wb') as main_file: + with mg_globals.public_store.get_file(main_filepath, 'wb') as main_file: main_file.write(queued_file.read()) - queue_store.delete_file(queued_filepath) + mg_globals.queue_store.delete_file(queued_filepath) media_files_dict = entry.setdefault('media_files', {}) media_files_dict['thumb'] = thumb_filepath media_files_dict['main'] = main_filepath From a32acafa0bfebfc886a05414b4e33943d358efc7 Mon Sep 17 00:00:00 2001 From: Christopher Allan Webber Date: Sat, 11 Jun 2011 20:33:41 -0500 Subject: [PATCH 15/30] Moving workbench out of process_media --- mediagoblin/app.py | 3 +-- mediagoblin/celery_setup/from_celery.py | 3 +-- mediagoblin/tests/test_workbench.py | 2 +- mediagoblin/{process_media => }/workbench.py | 0 4 files changed, 3 insertions(+), 5 deletions(-) rename mediagoblin/{process_media => }/workbench.py (100%) diff --git a/mediagoblin/app.py b/mediagoblin/app.py index da2b7d35..5d594039 100644 --- a/mediagoblin/app.py +++ b/mediagoblin/app.py @@ -25,8 +25,7 @@ from mediagoblin import routing, util, storage, staticdirect from mediagoblin.db.open import setup_connection_and_db_from_config from mediagoblin.globals import setup_globals from mediagoblin.celery_setup import setup_celery_from_config -from mediagoblin.process_media.workbench import ( - WorkbenchManager, DEFAULT_WORKBENCH_DIR) +from mediagoblin.workbench import WorkbenchManager, DEFAULT_WORKBENCH_DIR class Error(Exception): pass diff --git a/mediagoblin/celery_setup/from_celery.py b/mediagoblin/celery_setup/from_celery.py index e1216815..5fa9ba76 100644 --- a/mediagoblin/celery_setup/from_celery.py +++ b/mediagoblin/celery_setup/from_celery.py @@ -23,8 +23,7 @@ from mediagoblin import storage from mediagoblin.db.open import setup_connection_and_db_from_config from mediagoblin.celery_setup import setup_celery_from_config from mediagoblin.globals import setup_globals -from mediagoblin.process_media.workbench import ( - WorkbenchManager, DEFAULT_WORKBENCH_DIR) +from mediagoblin.workbench import WorkbenchManager, DEFAULT_WORKBENCH_DIR OUR_MODULENAME = 'mediagoblin.celery_setup.from_celery' diff --git a/mediagoblin/tests/test_workbench.py b/mediagoblin/tests/test_workbench.py index fd71a6eb..f08a26a0 100644 --- a/mediagoblin/tests/test_workbench.py +++ b/mediagoblin/tests/test_workbench.py @@ -19,7 +19,7 @@ import tempfile from nose.tools import assert_raises -from mediagoblin.process_media import workbench +from mediagoblin import workbench from mediagoblin.tests.test_storage import get_tmp_filestorage diff --git a/mediagoblin/process_media/workbench.py b/mediagoblin/workbench.py similarity index 100% rename from mediagoblin/process_media/workbench.py rename to mediagoblin/workbench.py From fdc5003903584ec8a1e83ccf80ebb6d3be3c671e Mon Sep 17 00:00:00 2001 From: Christopher Allan Webber Date: Sat, 11 Jun 2011 21:20:26 -0500 Subject: [PATCH 16/30] Don't bother returning whether or not we copied it or not, we can figure that out by looking to see whether our storage is local or not. --- mediagoblin/tests/test_workbench.py | 9 ++++----- mediagoblin/workbench.py | 19 +++++++------------ 2 files changed, 11 insertions(+), 17 deletions(-) diff --git a/mediagoblin/tests/test_workbench.py b/mediagoblin/tests/test_workbench.py index f08a26a0..994688c4 100644 --- a/mediagoblin/tests/test_workbench.py +++ b/mediagoblin/tests/test_workbench.py @@ -65,9 +65,8 @@ class TestWorkbench(object): our_file.write('Our file') # with a local file storage - filename, copied = self.workbench_manager.possibly_localize_file( + filename = self.workbench_manager.possibly_localize_file( this_workbench, this_storage, filepath) - assert copied is False assert filename == os.path.join( tmpdir, 'dir1/dir2/ourfile.txt') @@ -78,20 +77,20 @@ class TestWorkbench(object): with this_storage.get_file(filepath, 'w') as our_file: our_file.write('Our file') - filename, copied = self.workbench_manager.possibly_localize_file( + filename = self.workbench_manager.possibly_localize_file( this_workbench, this_storage, filepath) assert filename == os.path.join( this_workbench, 'ourfile.txt') # fake remote file storage, filename_if_copying set - filename, copied = self.workbench_manager.possibly_localize_file( + filename = self.workbench_manager.possibly_localize_file( this_workbench, this_storage, filepath, 'thisfile') assert filename == os.path.join( this_workbench, 'thisfile.txt') # fake remote file storage, filename_if_copying set, # keep_extension_if_copying set to false - filename, copied = self.workbench_manager.possibly_localize_file( + filename = self.workbench_manager.possibly_localize_file( this_workbench, this_storage, filepath, 'thisfile.text', False) assert filename == os.path.join( this_workbench, 'thisfile.text') diff --git a/mediagoblin/workbench.py b/mediagoblin/workbench.py index b1198adf..360e3e19 100644 --- a/mediagoblin/workbench.py +++ b/mediagoblin/workbench.py @@ -89,37 +89,32 @@ class WorkbenchManager(object): filename_if_copying yourself, it'll be set for you (assuming such an extension can be extacted from the filename in the filepath). - Also returns whether or not it copied the file locally. - Returns: - (localized_filename, copied_locally) - The first of these bieng the absolute filename as described above as a - unicode string, the second being a boolean stating whether or not we - had to copy the file locally. + localized_filename Examples: >>> wb_manager.possibly_localize_file( ... '/our/workbench/subdir', local_storage, ... ['path', 'to', 'foobar.jpg']) - (u'/local/storage/path/to/foobar.jpg', False) + u'/local/storage/path/to/foobar.jpg' >>> wb_manager.possibly_localize_file( ... '/our/workbench/subdir', remote_storage, ... ['path', 'to', 'foobar.jpg']) - (u'/our/workbench/subdir/foobar.jpg', True) + '/our/workbench/subdir/foobar.jpg' >>> wb_manager.possibly_localize_file( ... '/our/workbench/subdir', remote_storage, ... ['path', 'to', 'foobar.jpg'], 'source.jpeg', False) - (u'/our/workbench/subdir/foobar.jpeg', True) + '/our/workbench/subdir/foobar.jpeg' >>> wb_manager.possibly_localize_file( ... '/our/workbench/subdir', remote_storage, ... ['path', 'to', 'foobar.jpg'], 'source', True) - (u'/our/workbench/subdir/foobar.jpg', True) + '/our/workbench/subdir/foobar.jpg' """ if storage.local_storage: - return (storage.get_local_path(filepath), False) + return storage.get_local_path(filepath) else: if filename_if_copying is None: dest_filename = filepath[-1] @@ -137,4 +132,4 @@ class WorkbenchManager(object): storage.copy_locally( filepath, full_dest_filename) - return (full_dest_filename, True) + return full_dest_filename From ca030ab6cd5c18b7991f08a1ee103175f313c5a1 Mon Sep 17 00:00:00 2001 From: Christopher Allan Webber Date: Sat, 11 Jun 2011 21:20:39 -0500 Subject: [PATCH 17/30] Switch process_media over to using the workbench. --- mediagoblin/process_media/__init__.py | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/mediagoblin/process_media/__init__.py b/mediagoblin/process_media/__init__.py index 527c198c..1bb43755 100644 --- a/mediagoblin/process_media/__init__.py +++ b/mediagoblin/process_media/__init__.py @@ -26,11 +26,17 @@ THUMB_SIZE = 200, 200 @task def process_media_initial(media_id): + workbench = mg_globals.workbench_manager.create_workbench() + entry = mg_globals.database.MediaEntry.one( {'_id': ObjectId(media_id)}) queued_filepath = entry['queued_media_file'] - queued_file = mg_globals.queue_store.get_file(queued_filepath, 'r') + queued_filename = mg_globals.workbench_manager.possibly_localize_file( + workbench, mg_globals.queue_store, queued_filepath, + 'source') + + queued_file = file(queued_filename, 'r') with queued_file: thumb = Image.open(queued_file) @@ -47,7 +53,7 @@ def process_media_initial(media_id): # we have to re-read because unlike PIL, not everything reads # things in string representation :) - queued_file = mg_globals.queue_store.get_file(queued_filepath, 'rb') + queued_file = file(queued_filename, 'rb') with queued_file: main_filepath = mg_globals.public_store.get_unique_filepath( @@ -64,3 +70,6 @@ def process_media_initial(media_id): media_files_dict['main'] = main_filepath entry['state'] = u'processed' entry.save() + + # clean up workbench + mg_globals.workbench_manager.destroy_workbench(workbench) From 68ffb13690fa0c364c514ce253364f928e50841c Mon Sep 17 00:00:00 2001 From: Christopher Allan Webber Date: Sat, 11 Jun 2011 21:23:32 -0500 Subject: [PATCH 18/30] possibly_localize_file->localized_file... a bit less terribly long. --- mediagoblin/process_media/__init__.py | 2 +- mediagoblin/tests/test_workbench.py | 10 +++++----- mediagoblin/workbench.py | 14 +++++++------- 3 files changed, 13 insertions(+), 13 deletions(-) diff --git a/mediagoblin/process_media/__init__.py b/mediagoblin/process_media/__init__.py index 1bb43755..531eb16d 100644 --- a/mediagoblin/process_media/__init__.py +++ b/mediagoblin/process_media/__init__.py @@ -32,7 +32,7 @@ def process_media_initial(media_id): {'_id': ObjectId(media_id)}) queued_filepath = entry['queued_media_file'] - queued_filename = mg_globals.workbench_manager.possibly_localize_file( + queued_filename = mg_globals.workbench_manager.localized_file( workbench, mg_globals.queue_store, queued_filepath, 'source') diff --git a/mediagoblin/tests/test_workbench.py b/mediagoblin/tests/test_workbench.py index 994688c4..89f2ef33 100644 --- a/mediagoblin/tests/test_workbench.py +++ b/mediagoblin/tests/test_workbench.py @@ -54,7 +54,7 @@ class TestWorkbench(object): self.workbench_manager.destroy_workbench, dont_kill_this) - def test_possibly_localize_file(self): + def test_localized_file(self): tmpdir, this_storage = get_tmp_filestorage() this_workbench = self.workbench_manager.create_workbench() @@ -65,7 +65,7 @@ class TestWorkbench(object): our_file.write('Our file') # with a local file storage - filename = self.workbench_manager.possibly_localize_file( + filename = self.workbench_manager.localized_file( this_workbench, this_storage, filepath) assert filename == os.path.join( tmpdir, 'dir1/dir2/ourfile.txt') @@ -77,20 +77,20 @@ class TestWorkbench(object): with this_storage.get_file(filepath, 'w') as our_file: our_file.write('Our file') - filename = self.workbench_manager.possibly_localize_file( + filename = self.workbench_manager.localized_file( this_workbench, this_storage, filepath) assert filename == os.path.join( this_workbench, 'ourfile.txt') # fake remote file storage, filename_if_copying set - filename = self.workbench_manager.possibly_localize_file( + filename = self.workbench_manager.localized_file( this_workbench, this_storage, filepath, 'thisfile') assert filename == os.path.join( this_workbench, 'thisfile.txt') # fake remote file storage, filename_if_copying set, # keep_extension_if_copying set to false - filename = self.workbench_manager.possibly_localize_file( + filename = self.workbench_manager.localized_file( this_workbench, this_storage, filepath, 'thisfile.text', False) assert filename == os.path.join( this_workbench, 'thisfile.text') diff --git a/mediagoblin/workbench.py b/mediagoblin/workbench.py index 360e3e19..d7252623 100644 --- a/mediagoblin/workbench.py +++ b/mediagoblin/workbench.py @@ -70,9 +70,9 @@ class WorkbenchManager(object): shutil.rmtree(workbench) - def possibly_localize_file(self, workbench, storage, filepath, - filename_if_copying=None, - keep_extension_if_copying=True): + def localized_file(self, workbench, storage, filepath, + filename_if_copying=None, + keep_extension_if_copying=True): """ Possibly localize the file from this storage system (for read-only purposes, modifications should be written to a new file.). @@ -93,22 +93,22 @@ class WorkbenchManager(object): localized_filename Examples: - >>> wb_manager.possibly_localize_file( + >>> wb_manager.localized_file( ... '/our/workbench/subdir', local_storage, ... ['path', 'to', 'foobar.jpg']) u'/local/storage/path/to/foobar.jpg' - >>> wb_manager.possibly_localize_file( + >>> wb_manager.localized_file( ... '/our/workbench/subdir', remote_storage, ... ['path', 'to', 'foobar.jpg']) '/our/workbench/subdir/foobar.jpg' - >>> wb_manager.possibly_localize_file( + >>> wb_manager.localized_file( ... '/our/workbench/subdir', remote_storage, ... ['path', 'to', 'foobar.jpg'], 'source.jpeg', False) '/our/workbench/subdir/foobar.jpeg' - >>> wb_manager.possibly_localize_file( + >>> wb_manager.localized_file( ... '/our/workbench/subdir', remote_storage, ... ['path', 'to', 'foobar.jpg'], 'source', True) '/our/workbench/subdir/foobar.jpg' From 6e7ce8d1af8c6fcf7d00992b1c8ef0e8c1602479 Mon Sep 17 00:00:00 2001 From: Christopher Allan Webber Date: Sun, 12 Jun 2011 17:27:37 -0500 Subject: [PATCH 19/30] mediagoblin.globals->mediagoblin.mg_globals --- mediagoblin/app.py | 2 +- mediagoblin/auth/lib.py | 4 ++-- mediagoblin/celery_setup/from_celery.py | 2 +- mediagoblin/db/migrations.py | 2 -- mediagoblin/db/models.py | 4 ++-- mediagoblin/gmg_commands/migrate.py | 1 - mediagoblin/gmg_commands/shell.py | 8 ++++---- mediagoblin/{globals.py => mg_globals.py} | 2 +- mediagoblin/process_media/__init__.py | 2 +- mediagoblin/tests/__init__.py | 4 ++-- mediagoblin/tests/test_auth.py | 10 +++++----- mediagoblin/tests/test_globals.py | 2 +- mediagoblin/tests/test_tests.py | 10 +++++----- mediagoblin/util.py | 14 +++++++------- 14 files changed, 32 insertions(+), 35 deletions(-) rename mediagoblin/{globals.py => mg_globals.py} (92%) diff --git a/mediagoblin/app.py b/mediagoblin/app.py index 5d594039..a1c6b512 100644 --- a/mediagoblin/app.py +++ b/mediagoblin/app.py @@ -23,7 +23,7 @@ from webob import Request, exc from mediagoblin import routing, util, storage, staticdirect from mediagoblin.db.open import setup_connection_and_db_from_config -from mediagoblin.globals import setup_globals +from mediagoblin.mg_globals import setup_globals from mediagoblin.celery_setup import setup_celery_from_config from mediagoblin.workbench import WorkbenchManager, DEFAULT_WORKBENCH_DIR diff --git a/mediagoblin/auth/lib.py b/mediagoblin/auth/lib.py index f40e560f..08bbdd16 100644 --- a/mediagoblin/auth/lib.py +++ b/mediagoblin/auth/lib.py @@ -20,7 +20,7 @@ import random import bcrypt from mediagoblin.util import send_email, render_template -from mediagoblin import globals as mgoblin_globals +from mediagoblin import mg_globals def bcrypt_check_password(raw_pass, stored_hash, extra_salt=None): @@ -112,7 +112,7 @@ def send_verification_email(user, request): # TODO: There is no error handling in place send_email( - mgoblin_globals.email_sender_address, + mg_globals.email_sender_address, [user['email']], # TODO # Due to the distributed nature of GNU MediaGoblin, we should diff --git a/mediagoblin/celery_setup/from_celery.py b/mediagoblin/celery_setup/from_celery.py index 71f93d76..c8ccebc8 100644 --- a/mediagoblin/celery_setup/from_celery.py +++ b/mediagoblin/celery_setup/from_celery.py @@ -22,7 +22,7 @@ from paste.deploy.converters import asbool from mediagoblin import storage from mediagoblin.db.open import setup_connection_and_db_from_config from mediagoblin.celery_setup import setup_celery_from_config -from mediagoblin.globals import setup_globals +from mediagoblin.mg_globals import setup_globals from mediagoblin.workbench import WorkbenchManager, DEFAULT_WORKBENCH_DIR diff --git a/mediagoblin/db/migrations.py b/mediagoblin/db/migrations.py index d035b15b..f1f625b7 100644 --- a/mediagoblin/db/migrations.py +++ b/mediagoblin/db/migrations.py @@ -16,8 +16,6 @@ from mongokit import DocumentMigration -from mediagoblin import globals as mediagoblin_globals - class MediaEntryMigration(DocumentMigration): def allmigration01_uploader_to_reference(self): diff --git a/mediagoblin/db/models.py b/mediagoblin/db/models.py index 3da97a49..d77cf619 100644 --- a/mediagoblin/db/models.py +++ b/mediagoblin/db/models.py @@ -20,7 +20,7 @@ from mongokit import Document, Set from mediagoblin import util from mediagoblin.auth import lib as auth_lib -from mediagoblin import globals as mediagoblin_globals +from mediagoblin import mg_globals from mediagoblin.db import migrations from mediagoblin.db.util import ObjectId @@ -114,7 +114,7 @@ class MediaEntry(Document): def generate_slug(self): self['slug'] = util.slugify(self['title']) - duplicate = mediagoblin_globals.database.media_entries.find_one( + duplicate = mg_globals.database.media_entries.find_one( {'slug': self['slug']}) if duplicate: diff --git a/mediagoblin/gmg_commands/migrate.py b/mediagoblin/gmg_commands/migrate.py index e04fb343..3ce25701 100644 --- a/mediagoblin/gmg_commands/migrate.py +++ b/mediagoblin/gmg_commands/migrate.py @@ -17,7 +17,6 @@ from mediagoblin.db import migrations from mediagoblin.gmg_commands import util as commands_util -from mediagoblin import globals as mgoblin_globals def migrate_parser_setup(subparser): diff --git a/mediagoblin/gmg_commands/shell.py b/mediagoblin/gmg_commands/shell.py index 9c0259de..16caf398 100644 --- a/mediagoblin/gmg_commands/shell.py +++ b/mediagoblin/gmg_commands/shell.py @@ -17,7 +17,7 @@ import code -from mediagoblin import globals as mgoblin_globals +from mediagoblin import mg_globals from mediagoblin.gmg_commands import util as commands_util @@ -35,7 +35,7 @@ GNU MediaGoblin shell! ---------------------- Available vars: - mgoblin_app: instantiated mediagoblin application - - mgoblin_globals: mediagoblin.globals + - mg_globals: mediagoblin.globals - db: database instance """ @@ -50,5 +50,5 @@ def shell(args): banner=SHELL_BANNER, local={ 'mgoblin_app': mgoblin_app, - 'mgoblin_globals': mgoblin_globals, - 'db': mgoblin_globals.database}) + 'mg_globals': mg_globals, + 'db': mg_globals.database}) diff --git a/mediagoblin/globals.py b/mediagoblin/mg_globals.py similarity index 92% rename from mediagoblin/globals.py rename to mediagoblin/mg_globals.py index 80d1f01d..2fca3c0a 100644 --- a/mediagoblin/globals.py +++ b/mediagoblin/mg_globals.py @@ -27,7 +27,7 @@ translations = gettext.find( def setup_globals(**kwargs): - from mediagoblin import globals as mg_globals + from mediagoblin import mg_globals for key, value in kwargs.iteritems(): setattr(mg_globals, key, value) diff --git a/mediagoblin/process_media/__init__.py b/mediagoblin/process_media/__init__.py index 531eb16d..71a0a277 100644 --- a/mediagoblin/process_media/__init__.py +++ b/mediagoblin/process_media/__init__.py @@ -18,7 +18,7 @@ import Image from mediagoblin.db.util import ObjectId from celery.task import task -from mediagoblin import globals as mg_globals +from mediagoblin import mg_globals THUMB_SIZE = 200, 200 diff --git a/mediagoblin/tests/__init__.py b/mediagoblin/tests/__init__.py index 46c7fd69..e9e2a59a 100644 --- a/mediagoblin/tests/__init__.py +++ b/mediagoblin/tests/__init__.py @@ -14,7 +14,7 @@ # You should have received a copy of the GNU Affero General Public License # along with this program. If not, see . -from .. import globals +from mediagoblin import mg_globals def setup_package(): @@ -22,5 +22,5 @@ def setup_package(): def teardown_package(): print "Killing db ..." - globals.db_connection.drop_database(globals.database.name) + mg_globals.db_connection.drop_database(mg_globals.database.name) print "... done" diff --git a/mediagoblin/tests/test_auth.py b/mediagoblin/tests/test_auth.py index cdfeccab..3d569093 100644 --- a/mediagoblin/tests/test_auth.py +++ b/mediagoblin/tests/test_auth.py @@ -20,7 +20,7 @@ from nose.tools import assert_equal from mediagoblin.auth import lib as auth_lib from mediagoblin.tests.tools import setup_fresh_app -from mediagoblin import globals as mgoblin_globals +from mediagoblin import mg_globals from mediagoblin import util @@ -137,7 +137,7 @@ def test_register_views(test_app): u'Passwords must match.'] ## At this point there should be no users in the database ;) - assert not mgoblin_globals.database.User.find().count() + assert not mg_globals.database.User.find().count() # Successful register # ------------------- @@ -158,7 +158,7 @@ def test_register_views(test_app): 'mediagoblin/auth/register_success.html') ## Make sure user is in place - new_user = mgoblin_globals.database.User.find_one( + new_user = mg_globals.database.User.find_one( {'username': 'happygirl'}) assert new_user assert new_user['status'] == u'needs_email_verification' @@ -191,7 +191,7 @@ def test_register_views(test_app): context = util.TEMPLATE_TEST_CONTEXT[ 'mediagoblin/auth/verify_email.html'] assert context['verification_successful'] == False - new_user = mgoblin_globals.database.User.find_one( + new_user = mg_globals.database.User.find_one( {'username': 'happygirl'}) assert new_user assert new_user['status'] == u'needs_email_verification' @@ -203,7 +203,7 @@ def test_register_views(test_app): context = util.TEMPLATE_TEST_CONTEXT[ 'mediagoblin/auth/verify_email.html'] assert context['verification_successful'] == True - new_user = mgoblin_globals.database.User.find_one( + new_user = mg_globals.database.User.find_one( {'username': 'happygirl'}) assert new_user assert new_user['status'] == u'active' diff --git a/mediagoblin/tests/test_globals.py b/mediagoblin/tests/test_globals.py index 6d2e01da..59d217f3 100644 --- a/mediagoblin/tests/test_globals.py +++ b/mediagoblin/tests/test_globals.py @@ -14,7 +14,7 @@ # You should have received a copy of the GNU Affero General Public License # along with this program. If not, see . -from mediagoblin import globals as mg_globals +from mediagoblin import mg_globals def test_setup_globals(): mg_globals.setup_globals( diff --git a/mediagoblin/tests/test_tests.py b/mediagoblin/tests/test_tests.py index 3ecbfac7..8ac7f0a4 100644 --- a/mediagoblin/tests/test_tests.py +++ b/mediagoblin/tests/test_tests.py @@ -16,7 +16,7 @@ from mediagoblin.tests.tools import get_test_app -from mediagoblin import globals as mgoblin_globals +from mediagoblin import mg_globals def test_get_test_app_wipes_db(): @@ -24,15 +24,15 @@ def test_get_test_app_wipes_db(): Make sure we get a fresh database on every wipe :) """ get_test_app() - assert mgoblin_globals.database.User.find().count() == 0 + assert mg_globals.database.User.find().count() == 0 - new_user = mgoblin_globals.database.User() + new_user = mg_globals.database.User() new_user['username'] = u'lolcat' new_user['email'] = u'lol@cats.example.org' new_user['pw_hash'] = u'pretend_this_is_a_hash' new_user.save() - assert mgoblin_globals.database.User.find().count() == 1 + assert mg_globals.database.User.find().count() == 1 get_test_app() - assert mgoblin_globals.database.User.find().count() == 0 + assert mg_globals.database.User.find().count() == 0 diff --git a/mediagoblin/util.py b/mediagoblin/util.py index 64e21ca9..f29f8570 100644 --- a/mediagoblin/util.py +++ b/mediagoblin/util.py @@ -31,7 +31,7 @@ import translitcodec from paste.deploy.loadwsgi import NicerConfigParser from webob import Response, exc -from mediagoblin import globals as mgoblin_globals +from mediagoblin import mg_globals from mediagoblin.db.util import ObjectId @@ -102,8 +102,8 @@ def get_jinja_env(template_loader, locale): extensions=['jinja2.ext.i18n']) template_env.install_gettext_callables( - mgoblin_globals.translations.gettext, - mgoblin_globals.translations.ngettext) + mg_globals.translations.gettext, + mg_globals.translations.ngettext) if exists(locale): SETUP_JINJA_ENVS[locale] = template_env @@ -264,9 +264,9 @@ def send_email(from_addr, to_addrs, subject, message_body): - message_body: email body text """ # TODO: make a mock mhost if testing is enabled - if TESTS_ENABLED or mgoblin_globals.email_debug_mode: + if TESTS_ENABLED or mg_globals.email_debug_mode: mhost = FakeMhost() - elif not mgoblin_globals.email_debug_mode: + elif not mg_globals.email_debug_mode: mhost = smtplib.SMTP() mhost.connect() @@ -279,7 +279,7 @@ def send_email(from_addr, to_addrs, subject, message_body): if TESTS_ENABLED: EMAIL_TEST_INBOX.append(message) - if getattr(mgoblin_globals, 'email_debug_mode', False): + if getattr(mg_globals, 'email_debug_mode', False): print u"===== Email =====" print u"From address: %s" % message['From'] print u"To addresses: %s" % message['To'] @@ -393,7 +393,7 @@ def setup_gettext(locale): if exists(locale): SETUP_GETTEXTS[locale] = this_gettext - mgoblin_globals.setup_globals( + mg_globals.setup_globals( translations=this_gettext) From 300c34e8ce52f20ba789c22c26bdc78b9ecb2954 Mon Sep 17 00:00:00 2001 From: Christopher Allan Webber Date: Sun, 12 Jun 2011 17:28:54 -0500 Subject: [PATCH 20/30] First import of mg_globals as mgg, partly because I just wanted it to be clear that it's okay to do by doing it *somewhere* :) --- mediagoblin/process_media/__init__.py | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/mediagoblin/process_media/__init__.py b/mediagoblin/process_media/__init__.py index 71a0a277..c71c5dda 100644 --- a/mediagoblin/process_media/__init__.py +++ b/mediagoblin/process_media/__init__.py @@ -18,7 +18,7 @@ import Image from mediagoblin.db.util import ObjectId from celery.task import task -from mediagoblin import mg_globals +from mediagoblin import mg_globals as mgg THUMB_SIZE = 200, 200 @@ -26,14 +26,14 @@ THUMB_SIZE = 200, 200 @task def process_media_initial(media_id): - workbench = mg_globals.workbench_manager.create_workbench() + workbench = mgg.workbench_manager.create_workbench() - entry = mg_globals.database.MediaEntry.one( + entry = mgg.database.MediaEntry.one( {'_id': ObjectId(media_id)}) queued_filepath = entry['queued_media_file'] - queued_filename = mg_globals.workbench_manager.localized_file( - workbench, mg_globals.queue_store, queued_filepath, + queued_filename = mgg.workbench_manager.localized_file( + workbench, mgg.queue_store, queued_filepath, 'source') queued_file = file(queued_filename, 'r') @@ -42,12 +42,12 @@ def process_media_initial(media_id): thumb = Image.open(queued_file) thumb.thumbnail(THUMB_SIZE, Image.ANTIALIAS) - thumb_filepath = mg_globals.public_store.get_unique_filepath( + thumb_filepath = mgg.public_store.get_unique_filepath( ['media_entries', unicode(entry['_id']), 'thumbnail.jpg']) - thumb_file = mg_globals.public_store.get_file(thumb_filepath, 'w') + thumb_file = mgg.public_store.get_file(thumb_filepath, 'w') with thumb_file: thumb.save(thumb_file, "JPEG") @@ -56,15 +56,15 @@ def process_media_initial(media_id): queued_file = file(queued_filename, 'rb') with queued_file: - main_filepath = mg_globals.public_store.get_unique_filepath( + main_filepath = mgg.public_store.get_unique_filepath( ['media_entries', unicode(entry['_id']), queued_filepath[-1]]) - with mg_globals.public_store.get_file(main_filepath, 'wb') as main_file: + with mgg.public_store.get_file(main_filepath, 'wb') as main_file: main_file.write(queued_file.read()) - mg_globals.queue_store.delete_file(queued_filepath) + mgg.queue_store.delete_file(queued_filepath) media_files_dict = entry.setdefault('media_files', {}) media_files_dict['thumb'] = thumb_filepath media_files_dict['main'] = main_filepath @@ -72,4 +72,4 @@ def process_media_initial(media_id): entry.save() # clean up workbench - mg_globals.workbench_manager.destroy_workbench(workbench) + mgg.workbench_manager.destroy_workbench(workbench) From 34d35a23930815438a4e1d8b28ec17016fb938c0 Mon Sep 17 00:00:00 2001 From: cfdv Date: Sun, 12 Jun 2011 17:35:07 -0500 Subject: [PATCH 21/30] ensure color mode compatibility when making image thumbnails --- mediagoblin/process_media/__init__.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/mediagoblin/process_media/__init__.py b/mediagoblin/process_media/__init__.py index c71c5dda..f0a6e511 100644 --- a/mediagoblin/process_media/__init__.py +++ b/mediagoblin/process_media/__init__.py @@ -41,6 +41,9 @@ def process_media_initial(media_id): with queued_file: thumb = Image.open(queued_file) thumb.thumbnail(THUMB_SIZE, Image.ANTIALIAS) + # ensure color mode is compatible with jpg + if thumb.mode != "RGB": + thumb = thumb.convert("RGB") thumb_filepath = mgg.public_store.get_unique_filepath( ['media_entries', From 52426ae01f0439af2833656a74eda70a240d66ae Mon Sep 17 00:00:00 2001 From: Elrond Date: Mon, 13 Jun 2011 00:36:56 +0200 Subject: [PATCH 22/30] Create a Workbench class and use it everywhere. Some references to Workbench.dir look ugly, I'm happy to hear suggestions there. --- mediagoblin/process_media/__init__.py | 4 +- mediagoblin/tests/test_workbench.py | 30 ++++++---- mediagoblin/workbench.py | 83 +++++++++++++++++---------- 3 files changed, 72 insertions(+), 45 deletions(-) diff --git a/mediagoblin/process_media/__init__.py b/mediagoblin/process_media/__init__.py index f0a6e511..bd067e39 100644 --- a/mediagoblin/process_media/__init__.py +++ b/mediagoblin/process_media/__init__.py @@ -32,8 +32,8 @@ def process_media_initial(media_id): {'_id': ObjectId(media_id)}) queued_filepath = entry['queued_media_file'] - queued_filename = mgg.workbench_manager.localized_file( - workbench, mgg.queue_store, queued_filepath, + queued_filename = workbench.localized_file( + mgg.queue_store, queued_filepath, 'source') queued_file = file(queued_filename, 'r') diff --git a/mediagoblin/tests/test_workbench.py b/mediagoblin/tests/test_workbench.py index 89f2ef33..2795cd63 100644 --- a/mediagoblin/tests/test_workbench.py +++ b/mediagoblin/tests/test_workbench.py @@ -30,24 +30,31 @@ class TestWorkbench(object): def test_create_workbench(self): workbench = self.workbench_manager.create_workbench() - assert os.path.isdir(workbench) - assert workbench.startswith(self.workbench_manager.base_workbench_dir) + assert os.path.isdir(workbench.dir) + assert workbench.dir.startswith(self.workbench_manager.base_workbench_dir) + + def test_joinpath(self): + this_workbench = self.workbench_manager.create_workbench() + tmpname = this_workbench.joinpath('temp.txt') + assert tmpname == os.path.join(this_workbench.dir, 'temp.txt') + self.workbench_manager.destroy_workbench(this_workbench) def test_destroy_workbench(self): # kill a workbench this_workbench = self.workbench_manager.create_workbench() - tmpfile = file(os.path.join(this_workbench, 'temp.txt'), 'w') + tmpfile_name = this_workbench.joinpath('temp.txt') + tmpfile = file(tmpfile_name, 'w') with tmpfile: tmpfile.write('lollerskates') - assert os.path.exists(os.path.join(this_workbench, 'temp.txt')) + assert os.path.exists(tmpfile_name) self.workbench_manager.destroy_workbench(this_workbench) - assert not os.path.exists(os.path.join(this_workbench, 'temp.txt')) - assert not os.path.exists(this_workbench) + assert not os.path.exists(tmpfile_name) + assert not os.path.exists(this_workbench.dir) # make sure we can't kill other stuff though - dont_kill_this = tempfile.mkdtemp() + dont_kill_this = workbench.Workbench(tempfile.mkdtemp()) assert_raises( workbench.WorkbenchOutsideScope, @@ -65,8 +72,7 @@ class TestWorkbench(object): our_file.write('Our file') # with a local file storage - filename = self.workbench_manager.localized_file( - this_workbench, this_storage, filepath) + filename = this_workbench.localized_file(this_storage, filepath) assert filename == os.path.join( tmpdir, 'dir1/dir2/ourfile.txt') @@ -80,17 +86,17 @@ class TestWorkbench(object): filename = self.workbench_manager.localized_file( this_workbench, this_storage, filepath) assert filename == os.path.join( - this_workbench, 'ourfile.txt') + this_workbench.dir, 'ourfile.txt') # fake remote file storage, filename_if_copying set filename = self.workbench_manager.localized_file( this_workbench, this_storage, filepath, 'thisfile') assert filename == os.path.join( - this_workbench, 'thisfile.txt') + this_workbench.dir, 'thisfile.txt') # fake remote file storage, filename_if_copying set, # keep_extension_if_copying set to false filename = self.workbench_manager.localized_file( this_workbench, this_storage, filepath, 'thisfile.text', False) assert filename == os.path.join( - this_workbench, 'thisfile.text') + this_workbench.dir, 'thisfile.text') diff --git a/mediagoblin/workbench.py b/mediagoblin/workbench.py index d7252623..c88b686c 100644 --- a/mediagoblin/workbench.py +++ b/mediagoblin/workbench.py @@ -36,41 +36,24 @@ class WorkbenchOutsideScope(Exception): # Actual workbench stuff # ---------------------- -class WorkbenchManager(object): +class Workbench(object): """ - A system for generating and destroying workbenches. - - Workbenches are actually just subdirectories of a temporary storage space - for during the processing stage. + Represent the directory for the workbench """ + def __init__(self, dir): + self.dir = dir - def __init__(self, base_workbench_dir): - self.base_workbench_dir = os.path.abspath(base_workbench_dir) - if not os.path.exists(self.base_workbench_dir): - os.makedirs(self.base_workbench_dir) - - def create_workbench(self): - """ - Create and return the path to a new workbench (directory). - """ - return tempfile.mkdtemp(dir=self.base_workbench_dir) + def __unicode__(self): + return unicode(self.dir) + def __str__(self): + return str(self.dir) + def __repr__(self): + return repr(self.dir) - def destroy_workbench(self, workbench): - """ - Destroy this workbench! Deletes the directory and all its contents! + def joinpath(self, *args): + return os.path.join(self.dir, *args) - Makes sure the workbench actually belongs to this manager though. - """ - # just in case - workbench = os.path.abspath(workbench) - - if not workbench.startswith(self.base_workbench_dir): - raise WorkbenchOutsideScope( - "Can't destroy workbench outside the base workbench dir") - - shutil.rmtree(workbench) - - def localized_file(self, workbench, storage, filepath, + def localized_file(self, storage, filepath, filename_if_copying=None, keep_extension_if_copying=True): """ @@ -126,10 +109,48 @@ class WorkbenchManager(object): dest_filename = filename_if_copying full_dest_filename = os.path.join( - workbench, dest_filename) + self.dir, dest_filename) # copy it over storage.copy_locally( filepath, full_dest_filename) return full_dest_filename + + +class WorkbenchManager(object): + """ + A system for generating and destroying workbenches. + + Workbenches are actually just subdirectories of a temporary storage space + for during the processing stage. + """ + + def __init__(self, base_workbench_dir): + self.base_workbench_dir = os.path.abspath(base_workbench_dir) + if not os.path.exists(self.base_workbench_dir): + os.makedirs(self.base_workbench_dir) + + def create_workbench(self): + """ + Create and return the path to a new workbench (directory). + """ + return Workbench(tempfile.mkdtemp(dir=self.base_workbench_dir)) + + def destroy_workbench(self, workbench): + """ + Destroy this workbench! Deletes the directory and all its contents! + + Makes sure the workbench actually belongs to this manager though. + """ + # just in case + workbench = os.path.abspath(workbench.dir) + + if not workbench.startswith(self.base_workbench_dir): + raise WorkbenchOutsideScope( + "Can't destroy workbench outside the base workbench dir") + + shutil.rmtree(workbench) + + def localized_file(self, workbench, *args, **kwargs): + return workbench.localized_file(*args, **kwargs) From 909c639d07aee08e2a3cd039e6fe4ae397dce5a7 Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sun, 12 Jun 2011 22:42:10 -0400 Subject: [PATCH 23/30] Fixes git workflow * overhauls the docs so they're (hopefully) clearer on the git workflow * adds text about putting things in bugfix branches, documenting your work, and using the issue tracker * adds a contrived example that uses aliens --- docs/git.rst | 184 ++++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 154 insertions(+), 30 deletions(-) diff --git a/docs/git.rst b/docs/git.rst index 0db1dacf..ddbcf226 100644 --- a/docs/git.rst +++ b/docs/git.rst @@ -2,11 +2,17 @@ Git, Cloning and Patches ========================== -GNU MediaGoblin uses git for all our version control and we have -the repositories hosted on `Gitorious `_. +GNU MediaGoblin uses git for all our version control and we have the +repositories hosted on `Gitorious `_. We have +two repositories: -We have two repositories. One is for the project and the other is for -the project website. +* MediaGoblin software: http://gitorious.org/mediagoblin/mediagoblin +* MediaGoblin website: http://gitorious.org/mediagoblin/mediagoblin-website + +It's most likely you want to look at the software repository--not the +website one. + +The rest of this chapter talks about using the software repository. How to clone the project @@ -20,46 +26,164 @@ Do:: How to send in patches ====================== +**Tie your changes to issues in the issue tracker** + All patches should be tied to issues in the `issue tracker -`_. -That makes it a lot easier for everyone to track proposed changes and -make sure your hard work doesn't get dropped on the floor! +`_. That makes +it a lot easier for everyone to track proposed changes and make sure +your hard work doesn't get dropped on the floor! If there isn't an +issue for what you're working on, please create one. The better the +description of what it is you're trying to fix/implement, the better +everyone else is able to understand why you're doing what you're +doing. -If there isn't an issue for what you're working on, please create -one. The better the description of what it is you're trying to -fix/implement, the better everyone else is able to understand why -you're doing what you're doing. +**Use bugfix branches** -There are two ways you could send in a patch. +The best way to isolate your changes is to create a branch based off +of the MediaGoblin repository master branch, do the changes related to +that one issue there, and then let us know how to get it. + +It's much easier on us if you isolate your changes to a branch focused +on the issue. Then we don't have to sift through things. + +It's much easier on you if you isolate your changes to a branch +focused on the issue. Then when we merge your changes in, you just +have to do a ``git fetch`` and that's it. This is especially true if +we reject some of your changes, but accept others or otherwise tweak +your changes. + +Further, if you isolate your changes to a branch, then you can work on +multiple issues at the same time and they don't conflict with one +another. + +**Properly document your changes** + +Include comments in the code. + +Write comprehensive commit messages. The better your commit message +is at describing what you did and why, the easier it is for us to +quickly accept your patch. + +Write comprehensive comments in the issue tracker about what you're +doing and why. -How to send in a patch from a publicly available clone ------------------------------------------------------- +**How to send us your changes** -Add a comment to the issue you're working on with the following bits -of information: +There are three ways to let us know how to get it: -* the url for your clone -* the revs you want looked at -* any details, questions, or other things that should be known +1. (preferred) **push changes to publicly available git clone and let + us know where to find it** + + Push your branch to your publicly available git clone and add a + comment to the issue with the url for your clone and the branch to + look at. + +2. **attaching the patch files to the issue** + + Run:: + + git format-patch -o patches /master + + Then tar up the newly created ``patches`` directory and attach the + directory to the issue. -How to send in a patch if you don't have a publicly available clone -------------------------------------------------------------------- +Example workflow +================ +Here's an example workflow. -Assuming that the remote is our repository on gitorious and the branch -to compare against is master, do the following: -1. checkout the branch you did your work in -2. do:: +Contributing changes +-------------------- - git format-patch -o patches origin/master +Slartibartfast from the planet Magrathea far off in the universe has +decided that he is bored with fjords and wants to fix issue 42 and +send us the changes. -3. either: +Slartibartfast has cloned the MediaGoblin repository and his clone +lives on gitorious. - * tar up and attach the tarball to the issue you're working on, OR - * attach the patch files to the issue you're working on one at a - time +Slartibartfast works locally. The remote named ``origin`` points to +his clone on gitorious. The remote named ``gmg`` points to the +MediaGoblin repository. + +Slartibartfast does the following: + +1. Fetches the latest from the MediaGoblin repository:: + + git fetch --all -p + +2. Creates a branch from the tip of the MediaGoblin repository (the + remote is named ``gmg``) master branch called ``issue_42``:: + + git checkout -b issue_42 gmg/master + +3. Slartibartfast works hard on his changes in the ``issue_42`` + branch. When done, he wants to notify us that he has made changes + he wants us to see. + +4. Slartibartfast pushes his changes to his clone (the remote is named + ``origin``):: + + git push origin issue_42 + +5. Slartibartfast adds a comment to issue 42 with the url for his + repository and the name of the branch he put the code in. He also + explains what he did and why it addresses the issue. + + +Updating a contribution +----------------------- + +Slartibartfast brushes his hands off with the sense of accomplishment +that comes with the knowledge of a job well done. He stands, wanders +over to get a cup of water, then realizes that he forgot to run the +unit tests! + +He runs the unit tests and discovers there's a bug in the code! + +Then he does this: + +1. He checks out the ``issue_42`` branch:: + + git checkout issue_42 + +2. He fixes the bug and checks it into the ``issue_42`` branch. + +3. He pushes his changes to his clone (the remote is named ``origin``):: + + git push origin issue_42 + +4. He adds another comment to issue 42 explaining about the mistake + and how he fixed it and that he's pushed the new change to the + ``issue_42`` branch of his publicly available clone. + + +What happens next +----------------- + +Slartibartfast is once again happy with his work. He finds issue 42 +in the issue tracker and adds a comment saying he submitted a merge +request with his changes and explains what they are. + +Later, someone checks out his code and finds a problem with it. He +adds a comment to the issue tracker specifying the problem and asks +Slartibartfast to fix it. Slartibartfst goes through the above steps +again, fixes the issue, pushes it to his ``issue_42`` branch and adds +another comment to the issue tracker about how he fixed it. + +Later, someone checks out his code and is happy with it. Someone +pulls it into the master branch of the MediaGoblin repository and adds +another comment to the issue and probably closes the issue out. + +Slartibartfast is notified of this. Slartibartfast does a:: + + git fetch --all + +The changes show up in the ``master`` branch of the ``gmg`` remote. +Slartibartfast now deletes his ``issue_42`` branch because he doesn't +need it anymore. How to learn git From fb6924170d0cc6b8648627edebc794e82b0946d7 Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sun, 12 Jun 2011 22:46:25 -0400 Subject: [PATCH 24/30] Tweaks git workflow structure * minor tweaking of the headers of the git workflow to break things up and organize them a bit better --- docs/git.rst | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/docs/git.rst b/docs/git.rst index ddbcf226..9a44867a 100644 --- a/docs/git.rst +++ b/docs/git.rst @@ -23,10 +23,11 @@ Do:: git clone git://gitorious.org/mediagoblin/mediagoblin.git -How to send in patches -====================== +How to contribute changes +========================= -**Tie your changes to issues in the issue tracker** +Tie your changes to issues in the issue tracker +----------------------------------------------- All patches should be tied to issues in the `issue tracker `_. That makes @@ -37,7 +38,9 @@ description of what it is you're trying to fix/implement, the better everyone else is able to understand why you're doing what you're doing. -**Use bugfix branches** + +Use bugfix branches to make changes +----------------------------------- The best way to isolate your changes is to create a branch based off of the MediaGoblin repository master branch, do the changes related to @@ -56,7 +59,9 @@ Further, if you isolate your changes to a branch, then you can work on multiple issues at the same time and they don't conflict with one another. -**Properly document your changes** + +Properly document your changes +------------------------------ Include comments in the code. @@ -68,7 +73,8 @@ Write comprehensive comments in the issue tracker about what you're doing and why. -**How to send us your changes** +How to send us your changes +--------------------------- There are three ways to let us know how to get it: From 1e85d28e018e6dc0d4be9af82f221ac5450423bb Mon Sep 17 00:00:00 2001 From: Christopher Allan Webber Date: Mon, 13 Jun 2011 09:08:18 -0500 Subject: [PATCH 25/30] Already mentioned, but clarifying that branches should be localized to a feature/bugfix/issue. --- docs/git.rst | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/git.rst b/docs/git.rst index 9a44867a..6421e093 100644 --- a/docs/git.rst +++ b/docs/git.rst @@ -81,9 +81,9 @@ There are three ways to let us know how to get it: 1. (preferred) **push changes to publicly available git clone and let us know where to find it** - Push your branch to your publicly available git clone and add a - comment to the issue with the url for your clone and the branch to - look at. + Push your feature/bugfix/issue branch to your publicly available + git clone and add a comment to the issue with the url for your + clone and the branch to look at. 2. **attaching the patch files to the issue** From 03db7d6c7416010a8eadecf93037ea398e6e07db Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Mon, 13 Jun 2011 12:24:52 -0400 Subject: [PATCH 26/30] Updates version in docs --- docs/conf.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/conf.py b/docs/conf.py index fedaf33c..0e75a617 100644 --- a/docs/conf.py +++ b/docs/conf.py @@ -48,9 +48,9 @@ copyright = u'2011, Free Software Foundation, Inc and contributors' # built documents. # # The short X.Y version. -version = '0.0.1' +version = '0.0.2' # The full version, including alpha/beta/rc tags. -release = '0.0.1' +release = '0.0.2' # The language for content autogenerated by Sphinx. Refer to documentation # for a list of supported languages. From 6729d65a3246208b658a3780c7fd62e20c666aa0 Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Mon, 13 Jun 2011 12:31:23 -0400 Subject: [PATCH 27/30] Adds local toc sections * Some of our chapters are pretty long and this should make it much easier for a user to find what they're looking for and jumping to it. It's easier to read the section toc at the top of the chapter, than it is to read it in the sidebar. --- docs/codebase.rst | 4 ++++ docs/contributinghowto.rst | 4 ++++ docs/designdecisions.rst | 4 ++++ docs/git.rst | 4 ++++ docs/hackinghowto.rst | 4 ++++ 5 files changed, 20 insertions(+) diff --git a/docs/codebase.rst b/docs/codebase.rst index 4f5f215f..898eadfe 100644 --- a/docs/codebase.rst +++ b/docs/codebase.rst @@ -4,6 +4,10 @@ Codebase Documentation ======================== +.. contents:: Sections + :local: + + This chapter covers the libraries that GNU MediaGoblin uses as well as various recipes for getting things done. diff --git a/docs/contributinghowto.rst b/docs/contributinghowto.rst index e980a5e0..06d2814e 100644 --- a/docs/contributinghowto.rst +++ b/docs/contributinghowto.rst @@ -4,6 +4,10 @@ Contributing HOWTO ==================== +.. contents:: Sections + :local: + + .. _join-the-community-section: Join the community! diff --git a/docs/designdecisions.rst b/docs/designdecisions.rst index 50dfe3e8..afa1e26b 100644 --- a/docs/designdecisions.rst +++ b/docs/designdecisions.rst @@ -4,6 +4,10 @@ Design Decisions ================== +.. contents:: Sections + :local: + + This chapter talks a bit about design decisions. diff --git a/docs/git.rst b/docs/git.rst index 6421e093..8eb038b2 100644 --- a/docs/git.rst +++ b/docs/git.rst @@ -2,6 +2,10 @@ Git, Cloning and Patches ========================== +.. contents:: Sections + :local: + + GNU MediaGoblin uses git for all our version control and we have the repositories hosted on `Gitorious `_. We have two repositories: diff --git a/docs/hackinghowto.rst b/docs/hackinghowto.rst index a9aadb62..d8bb9330 100644 --- a/docs/hackinghowto.rst +++ b/docs/hackinghowto.rst @@ -4,6 +4,10 @@ Hacking HOWTO =============== +.. contents:: Sections + :local: + + So you want to hack on GNU MediaGoblin? ======================================= From a68ee5556e2cf78abd1e87546f8627ec07c1f89d Mon Sep 17 00:00:00 2001 From: Christopher Allan Webber Date: Mon, 13 Jun 2011 21:01:19 -0500 Subject: [PATCH 28/30] A super strict HTML cleaner method with mediocre tests. --- mediagoblin/tests/test_util.py | 19 +++++++++++++++++++ mediagoblin/util.py | 27 +++++++++++++++++++++++++++ 2 files changed, 46 insertions(+) diff --git a/mediagoblin/tests/test_util.py b/mediagoblin/tests/test_util.py index 7b00a074..75e28aca 100644 --- a/mediagoblin/tests/test_util.py +++ b/mediagoblin/tests/test_util.py @@ -103,3 +103,22 @@ def test_locale_to_lower_lower(): # crazy renditions. Useful? assert util.locale_to_lower_lower('en-US') == 'en-us' assert util.locale_to_lower_lower('en_us') == 'en-us' + + +def test_html_cleaner(): + # Remove images + result = util.clean_html( + '

Hi everybody! ' + '

\n' + '

:)

') + assert result == ( + '
' + '

Hi everybody!

\n' + '

:)

' + '
') + + # Remove evil javascript + result = util.clean_html( + '

innocent link!

') + assert result == ( + '

innocent link!

') diff --git a/mediagoblin/util.py b/mediagoblin/util.py index f29f8570..fc380f41 100644 --- a/mediagoblin/util.py +++ b/mediagoblin/util.py @@ -30,6 +30,7 @@ import jinja2 import translitcodec from paste.deploy.loadwsgi import NicerConfigParser from webob import Response, exc +from lxml.html.clean import Cleaner from mediagoblin import mg_globals from mediagoblin.db.util import ObjectId @@ -373,6 +374,32 @@ def read_config_file(conf_file): return mgoblin_conf +# A super strict version of the lxml.html cleaner class +HTML_CLEANER = Cleaner( + scripts=True, + javascript=True, + comments=True, + style=True, + links=True, + page_structure=True, + processing_instructions=True, + embedded=True, + frames=True, + forms=True, + annoying_tags=True, + allow_tags=[ + 'div', 'b', 'i', 'em', 'strong', 'p', 'ul', 'ol', 'li', 'a', 'br'], + remove_unknown_tags=False, # can't be used with allow_tags + safe_attrs_only=True, + add_nofollow=True, # for now + host_whitelist=(), + whitelist_tags=set([])) + + +def clean_html(html): + return HTML_CLEANER.clean_html(html) + + SETUP_GETTEXTS = {} def setup_gettext(locale): From 8bfa533f8b438e57a7950a8dcd02a275cbfa19df Mon Sep 17 00:00:00 2001 From: Elrond Date: Tue, 14 Jun 2011 20:01:39 +0200 Subject: [PATCH 29/30] Drop WorkbenchManager.localized_file() As Workbench has the localized_file() method, use this everywhere and drop the wrapper method from WorkbenchManager. The processing code already did that. --- mediagoblin/tests/test_workbench.py | 11 +++++------ mediagoblin/workbench.py | 3 --- 2 files changed, 5 insertions(+), 9 deletions(-) diff --git a/mediagoblin/tests/test_workbench.py b/mediagoblin/tests/test_workbench.py index 2795cd63..db27dfc6 100644 --- a/mediagoblin/tests/test_workbench.py +++ b/mediagoblin/tests/test_workbench.py @@ -83,20 +83,19 @@ class TestWorkbench(object): with this_storage.get_file(filepath, 'w') as our_file: our_file.write('Our file') - filename = self.workbench_manager.localized_file( - this_workbench, this_storage, filepath) + filename = this_workbench.localized_file(this_storage, filepath) assert filename == os.path.join( this_workbench.dir, 'ourfile.txt') # fake remote file storage, filename_if_copying set - filename = self.workbench_manager.localized_file( - this_workbench, this_storage, filepath, 'thisfile') + filename = this_workbench.localized_file( + this_storage, filepath, 'thisfile') assert filename == os.path.join( this_workbench.dir, 'thisfile.txt') # fake remote file storage, filename_if_copying set, # keep_extension_if_copying set to false - filename = self.workbench_manager.localized_file( - this_workbench, this_storage, filepath, 'thisfile.text', False) + filename = this_workbench.localized_file( + this_storage, filepath, 'thisfile.text', False) assert filename == os.path.join( this_workbench.dir, 'thisfile.text') diff --git a/mediagoblin/workbench.py b/mediagoblin/workbench.py index c88b686c..32229d2e 100644 --- a/mediagoblin/workbench.py +++ b/mediagoblin/workbench.py @@ -151,6 +151,3 @@ class WorkbenchManager(object): "Can't destroy workbench outside the base workbench dir") shutil.rmtree(workbench) - - def localized_file(self, workbench, *args, **kwargs): - return workbench.localized_file(*args, **kwargs) From b67a983a02363bd17a4e6a96e650e65aa2d4eb7a Mon Sep 17 00:00:00 2001 From: Elrond Date: Tue, 14 Jun 2011 20:39:14 +0200 Subject: [PATCH 30/30] Move destroy_workbench to Workbench class And add a lot of warnings, as the checks for "being part of the main Manager" are all gone. --- mediagoblin/process_media/__init__.py | 2 +- mediagoblin/tests/test_workbench.py | 15 +++------ mediagoblin/workbench.py | 45 ++++++++++++--------------- 3 files changed, 25 insertions(+), 37 deletions(-) diff --git a/mediagoblin/process_media/__init__.py b/mediagoblin/process_media/__init__.py index bd067e39..f37bf080 100644 --- a/mediagoblin/process_media/__init__.py +++ b/mediagoblin/process_media/__init__.py @@ -75,4 +75,4 @@ def process_media_initial(media_id): entry.save() # clean up workbench - mgg.workbench_manager.destroy_workbench(workbench) + workbench.destroy_self() diff --git a/mediagoblin/tests/test_workbench.py b/mediagoblin/tests/test_workbench.py index db27dfc6..953835a1 100644 --- a/mediagoblin/tests/test_workbench.py +++ b/mediagoblin/tests/test_workbench.py @@ -37,7 +37,7 @@ class TestWorkbench(object): this_workbench = self.workbench_manager.create_workbench() tmpname = this_workbench.joinpath('temp.txt') assert tmpname == os.path.join(this_workbench.dir, 'temp.txt') - self.workbench_manager.destroy_workbench(this_workbench) + this_workbench.destroy_self() def test_destroy_workbench(self): # kill a workbench @@ -49,17 +49,10 @@ class TestWorkbench(object): assert os.path.exists(tmpfile_name) - self.workbench_manager.destroy_workbench(this_workbench) + wb_dir = this_workbench.dir + this_workbench.destroy_self() assert not os.path.exists(tmpfile_name) - assert not os.path.exists(this_workbench.dir) - - # make sure we can't kill other stuff though - dont_kill_this = workbench.Workbench(tempfile.mkdtemp()) - - assert_raises( - workbench.WorkbenchOutsideScope, - self.workbench_manager.destroy_workbench, - dont_kill_this) + assert not os.path.exists(wb_dir) def test_localized_file(self): tmpdir, this_storage = get_tmp_filestorage() diff --git a/mediagoblin/workbench.py b/mediagoblin/workbench.py index 32229d2e..f83c4fa0 100644 --- a/mediagoblin/workbench.py +++ b/mediagoblin/workbench.py @@ -23,24 +23,21 @@ DEFAULT_WORKBENCH_DIR = os.path.join( tempfile.gettempdir(), u'mgoblin_workbench') -# Exception(s) -# ------------ - -class WorkbenchOutsideScope(Exception): - """ - Raised when a workbench is outside a WorkbenchManager scope. - """ - pass - - # Actual workbench stuff # ---------------------- class Workbench(object): """ Represent the directory for the workbench + + WARNING: DO NOT create Workbench objects on your own, + let the WorkbenchManager do that for you! """ def __init__(self, dir): + """ + WARNING: DO NOT create Workbench objects on your own, + let the WorkbenchManager do that for you! + """ self.dir = dir def __unicode__(self): @@ -117,6 +114,19 @@ class Workbench(object): return full_dest_filename + def destroy_self(self): + """ + Destroy this workbench! Deletes the directory and all its contents! + + WARNING: Does no checks for a sane value in self.dir! + """ + # just in case + workbench = os.path.abspath(self.dir) + + shutil.rmtree(workbench) + + del self.dir + class WorkbenchManager(object): """ @@ -136,18 +146,3 @@ class WorkbenchManager(object): Create and return the path to a new workbench (directory). """ return Workbench(tempfile.mkdtemp(dir=self.base_workbench_dir)) - - def destroy_workbench(self, workbench): - """ - Destroy this workbench! Deletes the directory and all its contents! - - Makes sure the workbench actually belongs to this manager though. - """ - # just in case - workbench = os.path.abspath(workbench.dir) - - if not workbench.startswith(self.base_workbench_dir): - raise WorkbenchOutsideScope( - "Can't destroy workbench outside the base workbench dir") - - shutil.rmtree(workbench)