From 02e99cbc807c1ed1d21aa9c52b4473c1ec7f0ab2 Mon Sep 17 00:00:00 2001
From: Jillian Vogel <jill@opencraft.com>
Date: Mon, 17 Jan 2022 16:24:17 +1030
Subject: [PATCH 01/14] fix: diff-cover argument typo

---
 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 3d4a3f38..a6b3c06e 100644
--- a/Makefile
+++ b/Makefile
@@ -88,7 +88,7 @@ test: clean ## Run tests and generate coverage report
 	${VENV_BIN}/coverage run ./manage.py test blockstore --settings=blockstore.settings.test
 	${VENV_BIN}/coverage html
 	${VENV_BIN}/coverage xml
-	${VENV_BIN}/diff-cover coverage.xml --html-report diff-cover.html --compare-branch origin/master
+	${VENV_BIN}/diff-cover coverage.xml --html-report diff-cover.html --compare-branch=origin/master
 
 easyserver: dev.up dev.provision  # Start and provision a Blockstore container and run the server until CTRL-C, then stop it
 	# Now run blockstore until the user hits CTRL-C:

From ab9752275ea084ee77392b9746f6d727c961b2db Mon Sep 17 00:00:00 2001
From: Jillian Vogel <jill@opencraft.com>
Date: Mon, 17 Jan 2022 16:21:27 +1030
Subject: [PATCH 02/14] refactor: update python API to work directly with
 models

because the REST API will be removed as part of a future change.

Also removes the unneeded force_browser_url method which was copied from
the openedx blockstore_api -- it will be removed from openedx when the
blockstore service is removed, and it's no longer needed.
---
 CHANGELOG.rst                                 |   8 +
 blockstore/__init__.py                        |   2 +-
 blockstore/apps/api/__init__.py               |  23 +-
 blockstore/apps/api/{models.py => data.py}    |  44 +-
 blockstore/apps/api/exceptions.py             |   8 +
 blockstore/apps/api/methods.py                | 549 ++++++++++--------
 .../apps/api/tests/test_blockstore_api.py     |  20 +-
 7 files changed, 378 insertions(+), 276 deletions(-)
 rename blockstore/apps/api/{models.py => data.py} (72%)

diff --git a/CHANGELOG.rst b/CHANGELOG.rst
index 5dd3452c..fe711fbf 100644
--- a/CHANGELOG.rst
+++ b/CHANGELOG.rst
@@ -27,3 +27,11 @@ _____
   This code has been copied over so it is easier to review the Python API
   implementation in development.
 * Make code into an installable package.
+
+[1.2.0] - 2021-01-25
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+Changed
+_______
+
+* Updates the Python API to use the models directly.
diff --git a/blockstore/__init__.py b/blockstore/__init__.py
index d15b6c77..f48ad496 100644
--- a/blockstore/__init__.py
+++ b/blockstore/__init__.py
@@ -2,4 +2,4 @@
 Blockstore is a system for storing educational content.
 """
 
-__version__ = '1.1.0'
+__version__ = '1.2.0'
diff --git a/blockstore/apps/api/__init__.py b/blockstore/apps/api/__init__.py
index 483e97c2..a78bde7c 100644
--- a/blockstore/apps/api/__init__.py
+++ b/blockstore/apps/api/__init__.py
@@ -5,15 +5,16 @@
 openedx.core.djangolib.blockstore_cache) together with these API methods for
 improved performance.
 """
-from .models import (
-    Collection,
-    Bundle,
-    Draft,
-    BundleFile,
-    DraftFile,
-    LinkReference,
-    LinkDetails,
-    DraftLinkDetails,
+from .data import (
+    CollectionData,
+    BundleData,
+    BundleVersionData,
+    DraftData,
+    BundleFileData,
+    DraftFileData,
+    Dependency,
+    BundleLinkData,
+    DraftLinkData,
 )
 from .methods import (
     # Collections:
@@ -44,14 +45,14 @@
     # Links:
     get_bundle_links,
     get_bundle_version_links,
-    # Misc:
-    force_browser_url,
 )
 from .exceptions import (
     BlockstoreException,
     CollectionNotFound,
     BundleNotFound,
+    BundleVersionNotFound,
     DraftNotFound,
+    DraftHasNoChangesToCommit,
     BundleFileNotFound,
     BundleStorageError,
 )
diff --git a/blockstore/apps/api/models.py b/blockstore/apps/api/data.py
similarity index 72%
rename from blockstore/apps/api/models.py
rename to blockstore/apps/api/data.py
index 8f2127ca..96b85110 100644
--- a/blockstore/apps/api/models.py
+++ b/blockstore/apps/api/data.py
@@ -7,6 +7,8 @@
 
 import attr
 
+from blockstore.apps.bundles.links import Dependency
+
 
 def _convert_to_uuid(value):
     if not isinstance(value, UUID):
@@ -15,7 +17,7 @@ def _convert_to_uuid(value):
 
 
 @attr.s(frozen=True)
-class Collection:
+class CollectionData:
     """
     Metadata about a blockstore collection
     """
@@ -24,7 +26,7 @@ class Collection:
 
 
 @attr.s(frozen=True)
-class Bundle:
+class BundleData:
     """
     Metadata about a blockstore bundle
     """
@@ -38,20 +40,34 @@ class Bundle:
 
 
 @attr.s(frozen=True)
-class Draft:
+class DraftData:
     """
     Metadata about a blockstore draft
     """
     uuid = attr.ib(type=UUID, converter=_convert_to_uuid)
     bundle_uuid = attr.ib(type=UUID, converter=_convert_to_uuid)
     name = attr.ib(type=str)
+    created_at = attr.ib(type=datetime, validator=attr.validators.instance_of(datetime))
     updated_at = attr.ib(type=datetime, validator=attr.validators.instance_of(datetime))
     files = attr.ib(type=dict)
     links = attr.ib(type=dict)
 
 
 @attr.s(frozen=True)
-class BundleFile:
+class BundleVersionData:
+    """
+    Metadata about a blockstore bundle version.
+    """
+    bundle_uuid = attr.ib(type=UUID, converter=_convert_to_uuid)
+    version = attr.ib(type=int, validator=attr.validators.instance_of(int))
+    change_description = attr.ib(type=str)
+    created_at = attr.ib(type=datetime, validator=attr.validators.instance_of(datetime))
+    files = attr.ib(type=dict)
+    links = attr.ib(type=dict)
+
+
+@attr.s(frozen=True)
+class BundleFileData:
     """
     Metadata about a file in a blockstore bundle or draft.
     """
@@ -62,7 +78,7 @@ class BundleFile:
 
 
 @attr.s(frozen=True)
-class DraftFile(BundleFile):
+class DraftFileData(BundleFileData):
     """
     Metadata about a file in a blockstore draft.
     """
@@ -70,27 +86,17 @@ class DraftFile(BundleFile):
 
 
 @attr.s(frozen=True)
-class LinkReference:
-    """
-    A pointer to a specific BundleVersion
-    """
-    bundle_uuid = attr.ib(type=UUID, converter=_convert_to_uuid)
-    version = attr.ib(type=int)
-    snapshot_digest = attr.ib(type=str)
-
-
-@attr.s(frozen=True)
-class LinkDetails:
+class BundleLinkData:
     """
     Details about a specific link in a BundleVersion or Draft
     """
     name = attr.ib(type=str)
-    direct = attr.ib(type=LinkReference)
-    indirect = attr.ib(type=list)  # List of LinkReference objects
+    direct = attr.ib(type=Dependency)
+    indirect = attr.ib(type=list)  # List of Dependency objects
 
 
 @attr.s(frozen=True)
-class DraftLinkDetails(LinkDetails):
+class DraftLinkData(BundleLinkData):
     """
     Details about a specific link in a Draft
     """
diff --git a/blockstore/apps/api/exceptions.py b/blockstore/apps/api/exceptions.py
index b58251d3..cc167687 100644
--- a/blockstore/apps/api/exceptions.py
+++ b/blockstore/apps/api/exceptions.py
@@ -19,10 +19,18 @@ class BundleNotFound(NotFound):
     pass
 
 
+class BundleVersionNotFound(NotFound):
+    pass
+
+
 class DraftNotFound(NotFound):
     pass
 
 
+class DraftHasNoChangesToCommit(BlockstoreException):
+    pass
+
+
 class BundleFileNotFound(NotFound):
     pass
 
diff --git a/blockstore/apps/api/methods.py b/blockstore/apps/api/methods.py
index a49d2d6d..c9e224a8 100644
--- a/blockstore/apps/api/methods.py
+++ b/blockstore/apps/api/methods.py
@@ -1,180 +1,99 @@
 """
 API Client methods for working with Blockstore bundles and drafts
 """
-# The code in this file has been copied over from edx-platform/openedx/core/lib/blockstore_api.
-# The following line should be removed when the implementation is being updated as part of
-# https://github.com/edx/blockstore/pull/97
-# pylint: skip-file
 
 import base64
-from urllib.parse import urlencode
-from uuid import UUID
-
-import dateutil.parser
-from django.conf import settings
-from django.core.exceptions import ImproperlyConfigured
-import requests
-
-from .models import (
-    Bundle,
-    Collection,
-    Draft,
-    BundleFile,
-    DraftFile,
-    LinkDetails,
-    LinkReference,
-    DraftLinkDetails,
+import crum
+
+from django.db.models import Q
+from rest_framework import serializers
+
+from blockstore.apps.bundles import models
+from blockstore.apps.bundles.links import LinkCycleError
+from blockstore.apps.bundles.store import DraftRepo, SnapshotRepo
+from blockstore.apps.rest_api.v1.serializers.drafts import (
+    DraftFileUpdateSerializer,
+)
+
+from .data import (
+    BundleData,
+    BundleVersionData,
+    CollectionData,
+    DraftData,
+    BundleFileData,
+    DraftFileData,
+    BundleLinkData,
+    DraftLinkData,
 )
 from .exceptions import (
-    NotFound,
     CollectionNotFound,
     BundleNotFound,
+    BundleVersionNotFound,
     DraftNotFound,
+    DraftHasNoChangesToCommit,
     BundleFileNotFound,
 )
 
 
-def api_url(*path_parts):
-    if not settings.BLOCKSTORE_API_URL or not settings.BLOCKSTORE_API_URL.endswith('/api/v1/'):
-        raise ImproperlyConfigured('BLOCKSTORE_API_URL must be set and should end with /api/v1/')
-    return settings.BLOCKSTORE_API_URL + '/'.join(path_parts)
-
-
-def api_request(method, url, **kwargs):
-    """
-    Helper method for making a request to the Blockstore REST API
-    """
-    if not settings.BLOCKSTORE_API_AUTH_TOKEN:
-        raise ImproperlyConfigured("Cannot use Blockstore unless BLOCKSTORE_API_AUTH_TOKEN is set.")
-    kwargs.setdefault('headers', {})['Authorization'] = f"Token {settings.BLOCKSTORE_API_AUTH_TOKEN}"
-    response = requests.request(method, url, **kwargs)
-    if response.status_code == 404:
-        raise NotFound
-    response.raise_for_status()
-    if response.status_code == 204:
-        return None  # No content
-    return response.json()
-
-
-def _collection_from_response(data):
-    """
-    Given data about a Collection returned by any blockstore REST API, convert it to
-    a Collection instance.
-    """
-    return Collection(uuid=UUID(data['uuid']), title=data['title'])
-
-
-def _bundle_from_response(data):
-    """
-    Given data about a Bundle returned by any blockstore REST API, convert it to
-    a Bundle instance.
-    """
-    return Bundle(
-        uuid=UUID(data['uuid']),
-        title=data['title'],
-        description=data['description'],
-        slug=data['slug'],
-        # drafts: Convert from a dict of URLs to a dict of UUIDs:
-        drafts={draft_name: UUID(url.split('/')[-1]) for (draft_name, url) in data['drafts'].items()},
-        # versions field: take the last one and convert it from URL to an int
-        # i.e.: [..., 'https://blockstore/api/v1/bundle_versions/bundle_uuid,15'] -> 15
-        latest_version=int(data['versions'][-1].split(',')[-1]) if data['versions'] else 0,
-    )
-
-
-def _draft_from_response(data):
-    """
-    Given data about a Draft returned by any blockstore REST API, convert it to
-    a Draft instance.
-    """
-    return Draft(
-        uuid=UUID(data['uuid']),
-        bundle_uuid=UUID(data['bundle_uuid']),
-        name=data['name'],
-        updated_at=dateutil.parser.parse(data['staged_draft']['updated_at']),
-        files={
-            path: DraftFile(path=path, **file)
-            for path, file in data['staged_draft']['files'].items()
-        },
-        links={
-            name: DraftLinkDetails(
-                name=name,
-                direct=LinkReference(**link["direct"]),
-                indirect=[LinkReference(**ind) for ind in link["indirect"]],
-                modified=link["modified"],
-            )
-            for name, link in data['staged_draft']['links'].items()
-        }
-    )
-
-
 def get_collection(collection_uuid):
     """
-    Retrieve metadata about the specified collection
+    Retrieve metadata about the specified collection.
 
-    Raises CollectionNotFound if the collection does not exist
+    Raises CollectionNotFound if collection with UUID does not exist.
     """
-    assert isinstance(collection_uuid, UUID)
-    try:
-        data = api_request('get', api_url('collections', str(collection_uuid)))
-    except NotFound:
-        raise CollectionNotFound(f"Collection {collection_uuid} does not exist.")
-    return _collection_from_response(data)
+    collection_model = _get_collection_model(collection_uuid)
+    return _collection_data_from_model(collection_model)
 
 
 def create_collection(title):
     """
     Create a new collection.
     """
-    result = api_request('post', api_url('collections'), json={"title": title})
-    return _collection_from_response(result)
+    collection_model = models.Collection(title=title)
+    collection_model.save()
+    return _collection_data_from_model(collection_model)
 
 
 def update_collection(collection_uuid, title):
     """
-    Update a collection's title
+    Update a collection's title.
     """
-    assert isinstance(collection_uuid, UUID)
-    data = {"title": title}
-    result = api_request('patch', api_url('collections', str(collection_uuid)), json=data)
-    return _collection_from_response(result)
+    collection_model = _get_collection_model(collection_uuid)
+    collection_model.title = title
+    collection_model.save()
+    return _collection_data_from_model(collection_model)
 
 
 def delete_collection(collection_uuid):
     """
-    Delete a collection
+    Delete a collection.
     """
-    assert isinstance(collection_uuid, UUID)
-    api_request('delete', api_url('collections', str(collection_uuid)))
+    collection_model = _get_collection_model(collection_uuid)
+    collection_model.delete()
 
 
 def get_bundles(uuids=None, text_search=None):
     """
-    Get the details of all bundles
+    Get the details of all bundles.
     """
-    query_params = {}
+    bundles_queryset = models.Bundle.objects.all()
     if uuids:
-        query_params['uuid'] = ','.join(map(str, uuids))
+        bundles_queryset = bundles_queryset.filter(uuid__in=uuids)
     if text_search:
-        query_params['text_search'] = text_search
-    version_url = api_url('bundles') + '?' + urlencode(query_params)
-    response = api_request('get', version_url)
-    # build bundle from response, convert map object to list and return
-    return [_bundle_from_response(item) for item in response]
+        bundles_queryset = bundles_queryset.filter(
+            Q(title__icontains=text_search) | Q(description__icontains=text_search) | Q(slug__icontains=text_search)
+        )
+    return [_bundle_data_from_model(bundle_model) for bundle_model in bundles_queryset]
 
 
 def get_bundle(bundle_uuid):
     """
-    Retrieve metadata about the specified bundle
+    Retrieve metadata about the specified bundle.
 
-    Raises BundleNotFound if the bundle does not exist
+    Raises BundleNotFound if bundle with UUID does not exist.
     """
-    assert isinstance(bundle_uuid, UUID)
-    try:
-        data = api_request('get', api_url('bundles', str(bundle_uuid)))
-    except NotFound:
-        raise BundleNotFound(f"Bundle {bundle_uuid} does not exist.")
-    return _bundle_from_response(data)
+    bundle_model = _get_bundle_model(bundle_uuid)
+    return _bundle_data_from_model(bundle_model)
 
 
 def create_bundle(collection_uuid, slug, title="New Bundle", description=""):
@@ -183,40 +102,42 @@ def create_bundle(collection_uuid, slug, title="New Bundle", description=""):
 
     Note that description is currently required.
     """
-    result = api_request('post', api_url('bundles'), json={
-        "collection_uuid": str(collection_uuid),
-        "slug": slug,
-        "title": title,
-        "description": description,
-    })
-    return _bundle_from_response(result)
+    collection_model = _get_collection_model(collection_uuid)
+    bundle_model = models.Bundle(
+        title=title,
+        collection=collection_model,
+        slug=slug,
+        description=description,
+    )
+    bundle_model.save()
+    return _bundle_data_from_model(bundle_model)
 
 
 def update_bundle(bundle_uuid, **fields):
     """
     Update a bundle's title, description, slug, or collection.
     """
-    assert isinstance(bundle_uuid, UUID)
-    data = {}
-    # Most validation will be done by Blockstore, so we don't worry too much about data validation
+    bundle_model = _get_bundle_model(bundle_uuid)
     for str_field in ("title", "description", "slug"):
         if str_field in fields:
-            data[str_field] = fields.pop(str_field)
+            setattr(bundle_model, str_field, fields.pop(str_field))
     if "collection_uuid" in fields:
-        data["collection_uuid"] = str(fields.pop("collection_uuid"))
+        collection_uuid = fields.pop("collection_uuid")
+        collection_model = _get_collection_model(collection_uuid)
+        bundle_model.collection = collection_model
     if fields:
-        raise ValueError(f"Unexpected extra fields passed "  # pylint: disable=dict-keys-not-iterating
-                         f"to update_bundle: {fields.keys()}")
-    result = api_request('patch', api_url('bundles', str(bundle_uuid)), json=data)
-    return _bundle_from_response(result)
+        raise ValueError("Unexpected extra fields passed to update_bundle: {}".format(fields.keys()))
+
+    bundle_model.save()
+    return _bundle_data_from_model(bundle_model)
 
 
 def delete_bundle(bundle_uuid):
     """
-    Delete a bundle
+    Delete a bundle.
     """
-    assert isinstance(bundle_uuid, UUID)
-    api_request('delete', api_url('bundles', str(bundle_uuid)))
+    bundle_model = _get_bundle_model(bundle_uuid)
+    bundle_model.delete()
 
 
 def get_draft(draft_uuid):
@@ -224,29 +145,24 @@ def get_draft(draft_uuid):
     Retrieve metadata about the specified draft.
     If you don't know the draft's UUID, look it up using get_bundle()
     """
-    assert isinstance(draft_uuid, UUID)
-    try:
-        data = api_request('get', api_url('drafts', str(draft_uuid)))
-    except NotFound:
-        raise DraftNotFound(f"Draft does not exist: {draft_uuid}")  # lint-amnesty, pylint: disable=raise-missing-from
-    return _draft_from_response(data)
+    draft_model = _get_draft_model(draft_uuid)
+    return _draft_data_from_model(draft_model)
 
 
 def get_or_create_bundle_draft(bundle_uuid, draft_name):
     """
-    Retrieve metadata about the specified draft.
+    Retrieve metadata about the specified draft, creating a new one if it does not exist yet.
     """
-    bundle = get_bundle(bundle_uuid)
     try:
-        return get_draft(bundle.drafts[draft_name])  # pylint: disable=unsubscriptable-object
-    except KeyError:
-        # The draft doesn't exist yet, so create it:
-        response = api_request('post', api_url('drafts'), json={
-            "bundle_uuid": str(bundle_uuid),
-            "name": draft_name,
-        })
-        # The result of creating a draft doesn't include all the fields we want, so retrieve it now:
-        return get_draft(UUID(response["uuid"]))
+        draft_model = models.Draft.objects.get(bundle__uuid=bundle_uuid, name=draft_name)
+    except models.Draft.DoesNotExist:
+        bundle_model = _get_bundle_model(bundle_uuid)
+        draft_model = models.Draft(
+            bundle=bundle_model,
+            name=draft_name,
+        )
+        draft_model.save()
+    return _draft_data_from_model(draft_model)
 
 
 def commit_draft(draft_uuid):
@@ -256,7 +172,16 @@ def commit_draft(draft_uuid):
 
     Does not return any value.
     """
-    api_request('post', api_url('drafts', str(draft_uuid), 'commit'))
+    draft_repo = DraftRepo(SnapshotRepo())
+    staged_draft = draft_repo.get(draft_uuid)
+
+    if not staged_draft.files_to_overwrite and not staged_draft.links_to_overwrite:
+        raise DraftHasNoChangesToCommit("Draft {} does not have any changes to commit.".format(draft_uuid))
+
+    new_snapshot, _updated_draft = draft_repo.commit(staged_draft)
+    models.BundleVersion.create_new_version(
+        new_snapshot.bundle_uuid, new_snapshot.hash_digest
+    )
 
 
 def delete_draft(draft_uuid):
@@ -265,17 +190,18 @@ def delete_draft(draft_uuid):
 
     Does not return any value.
     """
-    api_request('delete', api_url('drafts', str(draft_uuid)))
+    draft_model = _get_draft_model(draft_uuid)
+    draft_repo = DraftRepo(SnapshotRepo())
+    draft_repo.delete(draft_uuid)
+    draft_model.delete()
 
 
-def get_bundle_version(bundle_uuid, version_number):
+def get_bundle_version(bundle_uuid, version_number=None):
     """
     Get the details of the specified bundle version
     """
-    if version_number == 0:
-        return None
-    version_url = api_url('bundle_versions', str(bundle_uuid) + ',' + str(version_number))
-    return api_request('get', version_url)
+    bundle_version_model = _get_bundle_version_model(bundle_uuid, version_number)
+    return _bundle_version_data_from_model(bundle_version_model)
 
 
 def get_bundle_version_files(bundle_uuid, version_number):
@@ -284,8 +210,7 @@ def get_bundle_version_files(bundle_uuid, version_number):
     """
     if version_number == 0:
         return []
-    version_info = get_bundle_version(bundle_uuid, version_number)
-    return [BundleFile(path=path, **file_metadata) for path, file_metadata in version_info["snapshot"]["files"].items()]
+    return get_bundle_version(bundle_uuid, version_number).files.values()
 
 
 def get_bundle_version_links(bundle_uuid, version_number):
@@ -294,15 +219,7 @@ def get_bundle_version_links(bundle_uuid, version_number):
     """
     if version_number == 0:
         return {}
-    version_info = get_bundle_version(bundle_uuid, version_number)
-    return {
-        name: LinkDetails(
-            name=name,
-            direct=LinkReference(**link["direct"]),
-            indirect=[LinkReference(**ind) for ind in link["indirect"]],
-        )
-        for name, link in version_info['snapshot']['links'].items()
-    }
+    return get_bundle_version(bundle_uuid, version_number).links
 
 
 def get_bundle_files_dict(bundle_uuid, use_draft=None):
@@ -310,17 +227,17 @@ def get_bundle_files_dict(bundle_uuid, use_draft=None):
     Get a dict of all the files in the specified bundle.
 
     Returns a dict where the keys are the paths (strings) and the values are
-    BundleFile or DraftFile tuples.
-    """
-    bundle = get_bundle(bundle_uuid)
-    if use_draft and use_draft in bundle.drafts:  # pylint: disable=unsupported-membership-test
-        draft_uuid = bundle.drafts[use_draft]  # pylint: disable=unsubscriptable-object
-        return get_draft(draft_uuid).files
-    elif not bundle.latest_version:
-        # This bundle has no versions so definitely does not contain any files
-        return {}
-    else:
-        return {file_meta.path: file_meta for file_meta in get_bundle_version_files(bundle_uuid, bundle.latest_version)}
+    BundleFileData or DraftFileData tuples.
+    """
+    if use_draft:
+        try:
+            draft_model = models.Draft.objects.get(bundle__uuid=bundle_uuid, name=use_draft)
+        except models.Draft.DoesNotExist:
+            pass
+        else:
+            return _draft_data_from_model(draft_model).files
+
+    return get_bundle_version(bundle_uuid).files
 
 
 def get_bundle_files(bundle_uuid, use_draft=None):
@@ -337,29 +254,28 @@ def get_bundle_links(bundle_uuid, use_draft=None):
     Returns a dict where the keys are the link names (strings) and the values
     are LinkDetails or DraftLinkDetails tuples.
     """
-    bundle = get_bundle(bundle_uuid)
-    if use_draft and use_draft in bundle.drafts:  # pylint: disable=unsupported-membership-test
-        draft_uuid = bundle.drafts[use_draft]  # pylint: disable=unsubscriptable-object
-        return get_draft(draft_uuid).links
-    elif not bundle.latest_version:
-        # This bundle has no versions so definitely does not contain any links
-        return {}
-    else:
-        return get_bundle_version_links(bundle_uuid, bundle.latest_version)
+    if use_draft:
+        try:
+            draft_model = models.Draft.objects.get(bundle__uuid=bundle_uuid, name=use_draft)
+        except models.Draft.DoesNotExist:
+            pass
+        else:
+            return _draft_data_from_model(draft_model).links
+
+    return get_bundle_version(bundle_uuid).links
 
 
 def get_bundle_file_metadata(bundle_uuid, path, use_draft=None):
     """
     Get the metadata of the specified file.
     """
-    assert isinstance(bundle_uuid, UUID)
     files_dict = get_bundle_files_dict(bundle_uuid, use_draft=use_draft)
     try:
         return files_dict[path]
-    except KeyError:
-        raise BundleFileNotFound(  # lint-amnesty, pylint: disable=raise-missing-from
-            f"Bundle {bundle_uuid} (draft: {use_draft}) does not contain a file {path}"
-        )
+    except KeyError as exc:
+        raise BundleFileNotFound(
+            "Bundle {} (draft: {}) does not contain a file {}".format(bundle_uuid, use_draft, path)
+        ) from exc
 
 
 def get_bundle_file_data(bundle_uuid, path, use_draft=None):
@@ -369,9 +285,24 @@ def get_bundle_file_data(bundle_uuid, path, use_draft=None):
 
     Do not use this for large files!
     """
-    metadata = get_bundle_file_metadata(bundle_uuid, path, use_draft)
-    with requests.get(metadata.url, stream=True) as r:
-        return r.content
+
+    if use_draft:
+        try:
+            draft_model = models.Draft.objects.get(bundle__uuid=bundle_uuid, name=use_draft)
+        except models.Draft.DoesNotExist:
+            pass
+        else:
+            draft_repo = DraftRepo(SnapshotRepo())
+            staged_draft = draft_model.staged_draft
+            with draft_repo.open(staged_draft, path) as file:
+                return file.read()
+
+    bundle_version_model = _get_bundle_version_model(bundle_uuid)
+
+    snapshot_repo = SnapshotRepo()
+    snapshot = bundle_version_model.snapshot()
+    with snapshot_repo.open(snapshot, path) as file:
+        return file.read()
 
 
 def write_draft_file(draft_uuid, path, contents):
@@ -384,11 +315,21 @@ def write_draft_file(draft_uuid, path, contents):
 
     Does not return anything.
     """
-    api_request('patch', api_url('drafts', str(draft_uuid)), json={
+    data = {
         'files': {
-            path: encode_str_for_draft(contents) if contents is not None else None,
+            path: _encode_str_for_draft(contents) if contents is not None else None,
         },
-    })
+    }
+    serializer = DraftFileUpdateSerializer(data=data)
+    serializer.is_valid(raise_exception=True)
+    files_to_write = serializer.validated_data['files']
+    dependencies_to_write = serializer.validated_data['links']
+
+    draft_repo = DraftRepo(SnapshotRepo())
+    try:
+        draft_repo.update(draft_uuid, files_to_write, dependencies_to_write)
+    except LinkCycleError as exc:
+        raise serializers.ValidationError("Link cycle detected: Cannot create draft.") from exc
 
 
 def set_draft_link(draft_uuid, link_name, bundle_uuid, version):
@@ -402,14 +343,24 @@ def set_draft_link(draft_uuid, link_name, bundle_uuid, version):
 
     Does not return anything.
     """
-    api_request('patch', api_url('drafts', str(draft_uuid)), json={
+    data = {
         'links': {
             link_name: {"bundle_uuid": str(bundle_uuid), "version": version} if bundle_uuid is not None else None,
         },
-    })
+    }
+    serializer = DraftFileUpdateSerializer(data=data)
+    serializer.is_valid(raise_exception=True)
+    files_to_write = serializer.validated_data['files']
+    dependencies_to_write = serializer.validated_data['links']
 
+    draft_repo = DraftRepo(SnapshotRepo())
+    try:
+        draft_repo.update(draft_uuid, files_to_write, dependencies_to_write)
+    except LinkCycleError as exc:
+        raise serializers.ValidationError("Link cycle detected: Cannot create draft.") from exc
 
-def encode_str_for_draft(input_str):
+
+def _encode_str_for_draft(input_str):
     """
     Given a string, return UTF-8 representation that is then base64 encoded.
     """
@@ -420,18 +371,154 @@ def encode_str_for_draft(input_str):
     return base64.b64encode(binary)
 
 
-def force_browser_url(blockstore_file_url):
+def _get_collection_model(collection_uuid):
+    """
+    Get collection model from UUID.
+
+    Raises CollectionNotFound if the collection does not exist.
+    """
+    try:
+        collection_model = models.Collection.objects.get(uuid=collection_uuid)
+    except models.Collection.DoesNotExist as exc:
+        raise CollectionNotFound("Collection {} does not exist.".format(collection_uuid)) from exc
+    return collection_model
+
+
+def _collection_data_from_model(collection_model):
+    """
+    Create and return CollectionData from collection model.
+    """
+    return CollectionData(uuid=collection_model.uuid, title=collection_model.title)
+
+
+def _get_bundle_model(bundle_uuid):
+    """
+    Get Bundle model from UUID.
+
+    Raises BundleNotFound if bundle with UUID does not exist.
+    """
+    try:
+        bundle_model = models.Bundle.objects.get(uuid=bundle_uuid)
+    except models.Bundle.DoesNotExist as exc:
+        raise BundleNotFound("Bundle {} does not exist.".format(bundle_uuid)) from exc
+    return bundle_model
+
+
+def _bundle_data_from_model(bundle_model):
+    """
+    Create and return BundleData from bundle model.
+    """
+    latest_bundle_version_model = bundle_model.get_bundle_version()
+    return BundleData(
+        uuid=bundle_model.uuid,
+        title=bundle_model.title,
+        description=bundle_model.description,
+        slug=bundle_model.slug,
+        drafts={draft.name: draft.uuid for draft in bundle_model.drafts.all()},
+        latest_version=latest_bundle_version_model.version_num if latest_bundle_version_model else 0,
+    )
+
+
+def _get_draft_model(draft_uuid):
+    """
+    Get Draft model from UUID.
+
+    Raises DraftNotFound if draft with UUID does not exist.
+    """
+    try:
+        draft_model = models.Draft.objects.get(uuid=draft_uuid)
+    except models.Draft.DoesNotExist as exc:
+        raise DraftNotFound("Draft {} does not exist.".format(draft_uuid)) from exc
+    return draft_model
+
+
+def _build_absolute_uri(url):
+    """
+    Build an absolute URI from the given url, using the CRUM middleware's stored request.
+    """
+    request = crum.get_current_request()
+    return request.build_absolute_uri(url)
+
+
+def _draft_data_from_model(draft_model):
     """
-    Ensure that the given URL Blockstore is a URL accessible from the end user's
-    browser.
+    Create and return DraftData from draft model.
     """
-    # Hack: on some devstacks, we must necessarily use different URLs for
-    # accessing Blockstore file data from within and outside of docker
-    # containers, but Blockstore has no way of knowing which case any particular
-    # request is for. So it always returns a URL suitable for use from within
-    # the container. Only this edxapp can transform the URL at the last second,
-    # knowing that in this case it's going to the user's browser and not being
-    # read by edxapp.
-    # In production, the same S3 URLs get used for internal and external access
-    # so this hack is not necessary.
-    return blockstore_file_url.replace('http://edx.devstack.blockstore:', 'http://localhost:')
+    draft_repo = DraftRepo(SnapshotRepo())
+    staged_draft = draft_model.staged_draft
+
+    return DraftData(
+        uuid=draft_model.uuid,
+        bundle_uuid=draft_model.bundle.uuid,
+        name=draft_model.name,
+        created_at=draft_model.staged_draft.created_at,
+        updated_at=draft_model.staged_draft.updated_at,
+        files={
+            path: DraftFileData(
+                path=path,
+                size=file_info.size,
+                url=_build_absolute_uri(draft_repo.url(staged_draft, path)),
+                hash_digest=file_info.hash_digest,
+                modified=path in draft_model.staged_draft.files_to_overwrite,
+            )
+            for path, file_info in staged_draft.files.items()
+        },
+        links={
+            link.name: DraftLinkData(
+                name=link.name,
+                direct=link.direct_dependency,
+                indirect=link.indirect_dependencies,
+                modified=link.name in staged_draft.links_to_overwrite.modified_set,
+            )
+            for link in staged_draft.composed_links()
+        }
+    )
+
+
+def _get_bundle_version_model(bundle_uuid, version_number=None):
+    """
+    Get BundleVersion from bundle UUID and version number.
+
+    If version_number is None, returns the latest bundle version of the bundle.
+    """
+    filter_kwargs = {
+        'bundle__uuid': bundle_uuid
+    }
+    if version_number:
+        filter_kwargs['version_num'] = version_number
+
+    bundle_version_model = models.BundleVersion.objects.filter(**filter_kwargs).order_by('-version_num').first()
+    if bundle_version_model is None:
+        raise BundleVersionNotFound("Bundle Version {},{} does not exist.".format(bundle_uuid, version_number))
+    return bundle_version_model
+
+
+def _bundle_version_data_from_model(bundle_version_model):
+    """
+    Create and return BundleVersionData from bundle version model.
+    """
+    snapshot = bundle_version_model.snapshot()
+    snapshot_repo = SnapshotRepo()
+
+    return BundleVersionData(
+        bundle_uuid=bundle_version_model.bundle.uuid,
+        version=bundle_version_model.version_num,
+        change_description=bundle_version_model.change_description,
+        created_at=snapshot.created_at,
+        files={
+            path: BundleFileData(
+                path=path,
+                url=_build_absolute_uri(snapshot_repo.url(snapshot, path)),
+                size=file_info.size,
+                hash_digest=file_info.hash_digest.hex(),
+            ) for path, file_info in snapshot.files.items()
+        },
+        links={
+            link.name: BundleLinkData(
+                name=link.name,
+                direct=link.direct_dependency,
+                indirect=link.indirect_dependencies,
+            )
+            for link in snapshot.links
+        },
+    )
diff --git a/blockstore/apps/api/tests/test_blockstore_api.py b/blockstore/apps/api/tests/test_blockstore_api.py
index e512b796..2e97f228 100644
--- a/blockstore/apps/api/tests/test_blockstore_api.py
+++ b/blockstore/apps/api/tests/test_blockstore_api.py
@@ -1,16 +1,9 @@
 """
-Tests for xblock_utils.py
+Tests for the api.
 """
-# The code in this file has been copied over from edx-platform/openedx/core/lib/blockstore_api.
-# The following line should be removed when the implementation is being updated as part of
-# https://github.com/edx/blockstore/pull/97
-# pylint: skip-file
-
 import unittest
 from uuid import UUID
-
 import pytest
-from django.conf import settings
 
 from blockstore.apps import api
 
@@ -18,7 +11,6 @@
 BAD_UUID = UUID('12345678-0000-0000-0000-000000000000')
 
 
-@unittest.skip("Skip until the Python API has been implemented.")
 class BlockstoreApiClientTest(unittest.TestCase):
     """
     Test for the Blockstore API Client.
@@ -105,12 +97,12 @@ def test_drafts_and_files(self):
         coll = api.create_collection("Test Collection")
         bundle = api.create_bundle(coll.uuid, title="Earth 🗿 Bundle", slug="earth", description="another test bundle")
         # Create a draft
-        draft = api.get_or_create_draft(bundle.uuid, draft_name="test-draft")
+        draft = api.get_or_create_bundle_draft(bundle.uuid, draft_name="test-draft")
         assert draft.bundle_uuid == bundle.uuid
         assert draft.name == 'test-draft'
         assert draft.updated_at.year >= 2019
         # And retrieve it again:
-        draft2 = api.get_or_create_draft(bundle.uuid, draft_name="test-draft")
+        draft2 = api.get_or_create_bundle_draft(bundle.uuid, draft_name="test-draft")
         assert draft == draft2
         # Also test retrieving using get_draft
         draft3 = api.get_draft(draft.uuid)
@@ -153,11 +145,11 @@ def test_links(self):
         coll = api.create_collection("Test Collection")
         # Create two library bundles and a course bundle:
         lib1_bundle = api.create_bundle(coll.uuid, title="Library 1", slug="lib1")
-        lib1_draft = api.get_or_create_draft(lib1_bundle.uuid, draft_name="test-draft")
+        lib1_draft = api.get_or_create_bundle_draft(lib1_bundle.uuid, draft_name="test-draft")
         lib2_bundle = api.create_bundle(coll.uuid, title="Library 1", slug="lib2")
-        lib2_draft = api.get_or_create_draft(lib2_bundle.uuid, draft_name="other-draft")
+        lib2_draft = api.get_or_create_bundle_draft(lib2_bundle.uuid, draft_name="other-draft")
         course_bundle = api.create_bundle(coll.uuid, title="Library 1", slug="course")
-        course_draft = api.get_or_create_draft(course_bundle.uuid, draft_name="test-draft")
+        course_draft = api.get_or_create_bundle_draft(course_bundle.uuid, draft_name="test-draft")
 
         # To create links, we need valid BundleVersions, which requires having committed at least one change:
         api.write_draft_file(lib1_draft.uuid, "lib1-data.txt", "hello world")

From 070f5f302fd31e9a7a1d0147abd7348c26749e07 Mon Sep 17 00:00:00 2001
From: Jillian Vogel <jill@opencraft.com>
Date: Mon, 17 Jan 2022 18:17:17 +1030
Subject: [PATCH 03/14] test: adds unit tests to python API

to improve unit test coverage.
---
 CHANGELOG.rst                                 |   5 +
 .../apps/api/tests/test_blockstore_api.py     | 122 +++++++++++++++++-
 2 files changed, 125 insertions(+), 2 deletions(-)

diff --git a/CHANGELOG.rst b/CHANGELOG.rst
index fe711fbf..cca9ddf1 100644
--- a/CHANGELOG.rst
+++ b/CHANGELOG.rst
@@ -31,6 +31,11 @@ _____
 [1.2.0] - 2021-01-25
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
+Added
+_____
+
+* Adds API unit tests to improve test coverage.
+
 Changed
 _______
 
diff --git a/blockstore/apps/api/tests/test_blockstore_api.py b/blockstore/apps/api/tests/test_blockstore_api.py
index 2e97f228..a7c3a962 100644
--- a/blockstore/apps/api/tests/test_blockstore_api.py
+++ b/blockstore/apps/api/tests/test_blockstore_api.py
@@ -3,6 +3,8 @@
 """
 import unittest
 from uuid import UUID
+import crum
+from django.test.client import RequestFactory
 import pytest
 
 from blockstore.apps import api
@@ -19,6 +21,12 @@ class BlockstoreApiClientTest(unittest.TestCase):
     that the API client can interact with it and all the API client methods
     work.
     """
+    def setUp(self):
+        super().setUp()
+
+        # Mock the current request, so that file URLs can be absolute.
+        request = RequestFactory().get('/')
+        crum.set_current_request(request)
 
     # Collections
 
@@ -58,6 +66,7 @@ def test_nonexistent_bundle(self):
     def test_bundle_crud(self):
         """ Create, Fetch, Update, and Delete a Bundle """
         coll = api.create_collection("Test Collection")
+        coll2 = api.create_collection("Test Collection 2")
         args = {
             "title": "Water 💧 Bundle",
             "slug": "h2o",
@@ -73,7 +82,7 @@ def test_bundle_crud(self):
         assert bundle == bundle2
         # Update:
         new_description = "Water Nation Bending Lessons"
-        bundle3 = api.update_bundle(bundle.uuid, description=new_description)
+        bundle3 = api.update_bundle(bundle.uuid, description=new_description, collection_uuid=coll2.uuid)
         assert bundle3.description == new_description
         bundle4 = api.get_bundle(bundle.uuid)
         assert bundle4.description == new_description
@@ -82,6 +91,47 @@ def test_bundle_crud(self):
         with pytest.raises(api.BundleNotFound):
             api.get_bundle(bundle.uuid)
 
+    def test_get_bundles(self):
+        """ Fetch multiple bundles, and filter by text """
+        coll = api.create_collection("Test Collection")
+        water_bundle = api.create_bundle(
+            coll.uuid,
+            title="Water 💧 Bundle",
+            slug="h2o",
+            description="Sploosh",
+        )
+        air_bundle = api.create_bundle(
+            coll.uuid,
+            title="Air 🌀 Bundle",
+            slug="no2",
+            description="Whoosh",
+        )
+        fire_bundle = api.create_bundle(
+            coll.uuid,
+            title="Fire 🔥 Bundle",
+            slug="burn",
+            description="Crackle",
+        )
+
+        # Fetch multiple, and limit using text match
+        bundles = api.get_bundles([water_bundle.uuid, air_bundle.uuid, fire_bundle.uuid], text_search='oosh')
+        assert len(bundles) == 2
+        assert air_bundle in bundles
+        assert water_bundle in bundles
+        assert fire_bundle not in bundles
+
+    def test_update_bundle_invalid_fields(self):
+        """ Updating bundle with unexpected fields -> ValueError """
+        coll = api.create_collection("Test Collection")
+        bundle = api.create_bundle(
+            coll.uuid,
+            title="Water 💧 Bundle",
+            slug="h2o",
+            description="Sploosh",
+        )
+        with pytest.raises(ValueError):
+            api.update_bundle(bundle.uuid, invalid_field='Some invalid field value')
+
     # Drafts, files, and reading/writing file contents:
 
     def test_nonexistent_draft(self):
@@ -121,6 +171,14 @@ def test_drafts_and_files(self):
         assert published_contents == b'initial version'
         draft_contents2 = api.get_bundle_file_data(bundle.uuid, "test.txt", use_draft=draft.name)
         assert draft_contents2 == b'modified version'
+
+        file_info2 = api.get_bundle_file_metadata(bundle.uuid, "test.txt", use_draft=draft.name)
+        assert file_info2.url.startswith('http://testserver')
+        assert file_info2.path == 'test.txt'
+        assert file_info2.size == len(b'modified version')
+        assert file_info2.hash_digest == b'oM\xac\xfcD\xd2F\x11\xd2\xa7;\xff\x88\x8eS\x12\xc6\xe3\xfb\\'
+        assert api.get_bundle_files_dict(bundle.uuid, use_draft=draft.name) == {'test.txt': file_info2}
+
         # Now delete the draft:
         api.delete_draft(draft.uuid)
         draft_contents3 = api.get_bundle_file_data(bundle.uuid, "test.txt", use_draft=draft.name)
@@ -129,12 +187,63 @@ def test_drafts_and_files(self):
 
         # Finaly, test the get_bundle_file* methods:
         file_info1 = api.get_bundle_file_metadata(bundle.uuid, "test.txt")
+        assert file_info1.url.startswith('http://testserver')
         assert file_info1.path == 'test.txt'
         assert file_info1.size == len(b'initial version')
         assert file_info1.hash_digest == 'a45a5c6716276a66c4005534a51453ab16ea63c4'
 
         assert list(api.get_bundle_files(bundle.uuid)) == [file_info1]
         assert api.get_bundle_files_dict(bundle.uuid) == {'test.txt': file_info1}
+        assert api.get_bundle_files_dict(bundle.uuid, use_draft=draft.name) == {'test.txt': file_info1}
+
+        with pytest.raises(api.BundleFileNotFound):
+            api.get_bundle_file_metadata(bundle.uuid, "nonexistent.txt")
+
+    def test_bundle_version(self):
+        """
+        Test creating, reading, and writing bundle versions and files
+        """
+        coll = api.create_collection("Test Collection")
+        bundle = api.create_bundle(
+            coll.uuid,
+            title="Water 💧 Bundle",
+            slug="h2o",
+            description="Sploosh",
+        )
+
+        no_files = api.get_bundle_version_files(bundle.uuid, 0)
+        assert no_files == []
+
+        # Create and commit a draft
+        draft = api.get_or_create_bundle_draft(bundle.uuid, draft_name="test-draft")
+        api.commit_draft(draft.uuid)
+
+        # Fetch latest bundle version
+        latest_bundle_version = api.get_bundle_version(bundle.uuid)
+        bundle_version = api.get_bundle_version(bundle.uuid, 1)
+        assert bundle_version == latest_bundle_version
+        assert bundle_version.bundle_uuid == bundle.uuid
+        assert bundle_version.version == 1
+        assert bundle_version.files == {}
+        assert bundle_version.links == {}
+
+        # Unavailable version throws error
+        with pytest.raises(api.BundleVersionNotFound):
+            api.get_bundle_version(bundle.uuid, 2)
+
+        # Add file to a new draft
+        draft2 = api.get_or_create_bundle_draft(bundle.uuid, draft_name="test-draft-files")
+        api.write_draft_file(draft2.uuid, "test.txt", b"initial version")
+        api.commit_draft(draft2.uuid)
+
+        # Fetch this bundle version's files
+        files = api.get_bundle_version_files(bundle.uuid, 2)
+        assert len(files) == 1
+        for bundle_file in files:
+            assert bundle_file.url.startswith('http://testserver')
+            assert bundle_file.path == 'test.txt'
+            assert bundle_file.size == len(b'initial version')
+            assert bundle_file.hash_digest == 'a45a5c6716276a66c4005534a51453ab16ea63c4'
 
     # Links
 
@@ -150,6 +259,7 @@ def test_links(self):
         lib2_draft = api.get_or_create_bundle_draft(lib2_bundle.uuid, draft_name="other-draft")
         course_bundle = api.create_bundle(coll.uuid, title="Library 1", slug="course")
         course_draft = api.get_or_create_bundle_draft(course_bundle.uuid, draft_name="test-draft")
+        assert api.get_bundle_version_links(course_bundle.uuid, 0) == {}
 
         # To create links, we need valid BundleVersions, which requires having committed at least one change:
         api.write_draft_file(lib1_draft.uuid, "lib1-data.txt", "hello world")
@@ -185,6 +295,14 @@ def test_links(self):
         assert course_links[link2_name].indirect[0].bundle_uuid == lib1_bundle.uuid
         assert course_links[link2_name].indirect[0].version == 1
 
-        # Finally, test deleting a link from course's draft:
+        bundle_version_links = api.get_bundle_version_links(course_bundle.uuid, None)
+        assert bundle_version_links == course_links
+
+        # Test deleting a link from course's draft:
         api.set_draft_link(course_draft.uuid, link2_name, None, None)
         assert not api.get_bundle_links(course_bundle.uuid, use_draft=course_draft.name)
+
+        # Finally, delete the draft from the course and ensure the links are unchanged
+        api.delete_draft(course_draft.uuid)
+        course_links2 = api.get_bundle_links(course_bundle.uuid, use_draft=course_draft.name)
+        assert course_links == course_links2

From 01781bd8103d151ad6a07ababcf518abaaf5c191 Mon Sep 17 00:00:00 2001
From: Jillian Vogel <jill@opencraft.com>
Date: Wed, 19 Jan 2022 15:53:02 +1030
Subject: [PATCH 04/14] fix: allow tests using blockstore to run on sqlite

edx-platform unit tests run using sqlite, which doesn't support the
utf8mb4_general_ci db_collation setting. So when we're running pytest
with --no-migrations against a sqlite database, we use sqlite's default
collation setting.

Notes:

* These collation settings aren't supported by Postgres -- so
  everything documented in the bundles model hasn't been true since
  utf8mb4_general_ci was introduced.
* We're assuming the 'default' database, but that's dangerous -- db
  routers may choose different database types for the blockstore app.
---
 .../migrations/0003_update_character_set.py   | 15 +++++++------
 blockstore/apps/bundles/models.py             | 21 +++++++++++++------
 2 files changed, 24 insertions(+), 12 deletions(-)

diff --git a/blockstore/apps/bundles/migrations/0003_update_character_set.py b/blockstore/apps/bundles/migrations/0003_update_character_set.py
index afc59fb1..1e3e4555 100644
--- a/blockstore/apps/bundles/migrations/0003_update_character_set.py
+++ b/blockstore/apps/bundles/migrations/0003_update_character_set.py
@@ -3,6 +3,9 @@
 from django.db import migrations, models
 
 
+from blockstore.apps.bundles.models import DB_COLLATION
+
+
 class Migration(migrations.Migration):
 
     dependencies = [
@@ -13,31 +16,31 @@ class Migration(migrations.Migration):
         migrations.AlterField(
             model_name='bundle',
             name='description',
-            field=models.TextField(blank=True, db_collation='utf8mb4_general_ci', max_length=10000),
+            field=models.TextField(blank=True, db_collation=DB_COLLATION, max_length=10000),
         ),
         migrations.AlterField(
             model_name='bundle',
             name='slug',
-            field=models.SlugField(allow_unicode=True, db_collation='utf8mb4_general_ci'),
+            field=models.SlugField(allow_unicode=True, db_collation=DB_COLLATION),
         ),
         migrations.AlterField(
             model_name='bundle',
             name='title',
-            field=models.CharField(db_collation='utf8mb4_general_ci', db_index=True, max_length=180),
+            field=models.CharField(db_collation=DB_COLLATION, db_index=True, max_length=180),
         ),
         migrations.AlterField(
             model_name='bundleversion',
             name='change_description',
-            field=models.TextField(blank=True, db_collation='utf8mb4_general_ci', max_length=1000),
+            field=models.TextField(blank=True, db_collation=DB_COLLATION, max_length=1000),
         ),
         migrations.AlterField(
             model_name='collection',
             name='title',
-            field=models.CharField(db_collation='utf8mb4_general_ci', db_index=True, max_length=180),
+            field=models.CharField(db_collation=DB_COLLATION, db_index=True, max_length=180),
         ),
         migrations.AlterField(
             model_name='draft',
             name='name',
-            field=models.CharField(db_collation='utf8mb4_general_ci', max_length=180),
+            field=models.CharField(db_collation=DB_COLLATION, max_length=180),
         ),
     ]
diff --git a/blockstore/apps/bundles/models.py b/blockstore/apps/bundles/models.py
index 8685f4c6..faf875bb 100644
--- a/blockstore/apps/bundles/models.py
+++ b/blockstore/apps/bundles/models.py
@@ -54,6 +54,7 @@
 """
 import uuid
 
+from django.conf import settings
 from django.db import models
 
 from .store import DraftRepo, SnapshotRepo, bytes_from_hex_str
@@ -61,6 +62,14 @@
 MAX_CHAR_FIELD_LENGTH = 180
 
 
+"""
+Sqlite is used for testing on edx-platform, but doesn't contain the utf8mb4_general_ci collation sequence.
+
+So we detect this case here, and use a different db_collation if we're running on Sqlite.
+"""
+DB_COLLATION = 'binary' if 'sqlite' in settings.DATABASES['default']['ENGINE'] else 'utf8mb4_general_ci'
+
+
 class Collection(models.Model):
     """
     Administrative grouping for Bundles: policy, permissions, licensing, etc.
@@ -74,7 +83,7 @@ class Collection(models.Model):
     """
     id = models.BigAutoField(primary_key=True)
     uuid = models.UUIDField(default=uuid.uuid4, unique=True, editable=False)
-    title = models.CharField(max_length=MAX_CHAR_FIELD_LENGTH, db_index=True, db_collation='utf8mb4_general_ci')
+    title = models.CharField(max_length=MAX_CHAR_FIELD_LENGTH, db_index=True, db_collation=DB_COLLATION)
 
     def __str__(self):
         return f"{self.uuid} - {self.title}"
@@ -89,15 +98,15 @@ class Bundle(models.Model):
     """
     id = models.BigAutoField(primary_key=True)
     uuid = models.UUIDField(default=uuid.uuid4, unique=True, editable=False)
-    title = models.CharField(max_length=MAX_CHAR_FIELD_LENGTH, db_index=True, db_collation='utf8mb4_general_ci')
+    title = models.CharField(max_length=MAX_CHAR_FIELD_LENGTH, db_index=True, db_collation=DB_COLLATION)
 
     collection = models.ForeignKey(
         Collection, related_name="bundles", related_query_name="bundle", editable=False,
         on_delete=models.CASCADE,
     )
 
-    slug = models.SlugField(allow_unicode=True, db_collation='utf8mb4_general_ci')  # For pretty URLs
-    description = models.TextField(max_length=10000, blank=True, db_collation='utf8mb4_general_ci')
+    slug = models.SlugField(allow_unicode=True, db_collation=DB_COLLATION)  # For pretty URLs
+    description = models.TextField(max_length=10000, blank=True, db_collation=DB_COLLATION)
 
     def __str__(self):
         return f"Bundle {self.uuid} - {self.slug}"
@@ -132,7 +141,7 @@ class BundleVersion(models.Model):
     # This is a CharField only because Django ORM doens't support indexed binary
     # fields in MySQL
     snapshot_digest = models.CharField(max_length=40, db_index=True, editable=False)
-    change_description = models.TextField(max_length=1000, blank=True, db_collation='utf8mb4_general_ci')
+    change_description = models.TextField(max_length=1000, blank=True, db_collation=DB_COLLATION)
 
     class Meta:
         unique_together = (
@@ -197,7 +206,7 @@ class Draft(models.Model):
         Bundle, related_name="drafts", related_query_name="draft", editable=False,
         on_delete=models.CASCADE,
     )
-    name = models.CharField(max_length=MAX_CHAR_FIELD_LENGTH, db_collation='utf8mb4_general_ci')
+    name = models.CharField(max_length=MAX_CHAR_FIELD_LENGTH, db_collation=DB_COLLATION)
 
     def save(self, *args, **kwargs):
         if not self.pk:

From 32a315b3d76ad68d68f7a7a5d259986a501ee788 Mon Sep 17 00:00:00 2001
From: Jillian Vogel <jill@opencraft.com>
Date: Tue, 25 Jan 2022 17:27:10 +1030
Subject: [PATCH 05/14] feat: makes s3 asset storage configurable for the app

with storage settings separate from the global AWS settings.
---
 blockstore/apps/bundles/storage.py            | 17 ++++++++++----
 blockstore/apps/bundles/tests/test_storage.py | 15 ++++++++----
 blockstore/settings/base.py                   | 23 +++++++++++++++++--
 3 files changed, 44 insertions(+), 11 deletions(-)

diff --git a/blockstore/apps/bundles/storage.py b/blockstore/apps/bundles/storage.py
index 86f1adb3..62870ad0 100644
--- a/blockstore/apps/bundles/storage.py
+++ b/blockstore/apps/bundles/storage.py
@@ -90,11 +90,20 @@ def __init__(self):
             raise self.BackendNotAvailable from attr_error
         if not (key and secret):
             raise self.BackendNotAvailable
-        self.s3_backend = S3Boto3Storage(
-            # All other S3 settings will be pulled in automatically from Django settings
-            # (such as AWS_QUERYSTRING_EXPIRE and AWS_STORAGE_BUCKET_NAME).
-            access_key=key, secret_key=secret
+
+        # Provide the bucket name and prefix if configured;
+        # All other S3 settings will be pulled in automatically from Django settings
+        # (such as AWS_QUERYSTRING_EXPIRE and AWS_STORAGE_BUCKET_NAME).
+        s3_backend_args = dict(
+            access_key=key,
+            secret_key=secret,
         )
+        if settings.BUNDLE_ASSET_URL_STORAGE_BUCKET:
+            s3_backend_args['bucket_name'] = settings.BUNDLE_ASSET_URL_STORAGE_BUCKET
+        if settings.BUNDLE_ASSET_URL_STORAGE_PREFIX:
+            s3_backend_args['location'] = settings.BUNDLE_ASSET_URL_STORAGE_PREFIX
+
+        self.s3_backend = S3Boto3Storage(**s3_backend_args)
 
     def url(self, name):
         """
diff --git a/blockstore/apps/bundles/tests/test_storage.py b/blockstore/apps/bundles/tests/test_storage.py
index ec8b2acb..6bc48f12 100644
--- a/blockstore/apps/bundles/tests/test_storage.py
+++ b/blockstore/apps/bundles/tests/test_storage.py
@@ -14,11 +14,13 @@ class _MockS3backend:
     A fake replacment for S3Boto3Backend in these tests.
     """
     def __init__(self, **settings):
-        self.accesss_key = settings["access_key"]
+        self.access_key = settings["access_key"]
         self.secret_key = settings["secret_key"]
+        self.bucket_name = settings["bucket_name"]
+        self.location = settings["location"]
 
     def url(self, name):
-        return f"https://example.com/s3/{name}"
+        return f"https://{self.bucket_name}/{self.location}{name}"
 
 
 _patch_default_storage = patch.object(
@@ -28,7 +30,8 @@ def url(self, name):
     storage_module, 'get_storage_class', autospec=True, return_value=_MockS3backend
 )
 _patch_s3_credentials = override_settings(
-    BUNDLE_ASSET_URL_STORAGE_KEY="a-key", BUNDLE_ASSET_URL_STORAGE_SECRET="a-secret"
+    BUNDLE_ASSET_URL_STORAGE_KEY="a-key", BUNDLE_ASSET_URL_STORAGE_SECRET="a-secret",
+    BUNDLE_ASSET_URL_STORAGE_BUCKET='example-bucket', BUNDLE_ASSET_URL_STORAGE_PREFIX='s3/',
 )
 
 
@@ -58,9 +61,11 @@ def test_asset_storage_long_lived_urls_enabled(mock_default_storage, *_args):
     """
     backend = storage_module.AssetStorage()
     assert isinstance(backend.url_backend, storage_module.LongLivedSignedUrlStorage)
-    assert backend.url_backend.s3_backend.accesss_key == "a-key"
+    assert backend.url_backend.s3_backend.access_key == "a-key"
     assert backend.url_backend.s3_backend.secret_key == "a-secret"
-    assert backend.url('abc') == "https://example.com/s3/abc"
+    assert backend.url_backend.s3_backend.bucket_name == "example-bucket"
+    assert backend.url_backend.s3_backend.location == "s3/"
+    assert backend.url('abc') == "https://example-bucket/s3/abc"
     backend.listdir('123')
     backend.get_accessed_time('xyz')
     assert not mock_default_storage.url.called
diff --git a/blockstore/settings/base.py b/blockstore/settings/base.py
index 13da9875..5f4eacc0 100644
--- a/blockstore/settings/base.py
+++ b/blockstore/settings/base.py
@@ -302,11 +302,14 @@ def root(*x):
     }
 }
 
+################################################################################
+# BUNDLES CONFIGURATION
+
 # .. setting_name: BUNDLE_ASSET_URL_STORAGE_KEY
 # .. setting_default: None
 # .. setting_description: When this is set, `BUNDLE_ASSET_URL_STORAGE_SECRET` is
 #  set, and `boto3` is installed, this is used as an AWS IAM access key for
-#  generating signed, read-only URLs for assets stored in S3.
+#  generating signed, read-only URLs for blockstore assets stored in S3.
 #  Otherwise, URLs are generated based on the default storage configuration.
 #  See `blockstore.apps.bundles.storage.LongLivedSignedUrlStorage` for details.
 BUNDLE_ASSET_URL_STORAGE_KEY = None
@@ -315,7 +318,23 @@ def root(*x):
 # .. setting_default: None
 # .. setting_description: When this is set, `BUNDLE_ASSET_URL_STORAGE_KEY` is
 #  set, and `boto3` is installed, this is used as an AWS IAM secret key for
-#  generating signed, read-only URLs for assets stored in S3.
+#  generating signed, read-only URLs for blockstore assets stored in S3.
 #  Otherwise, URLs are generated based on the default storage configuration.
 #  See `blockstore.apps.bundles.storage.LongLivedSignedUrlStorage` for details.
 BUNDLE_ASSET_URL_STORAGE_SECRET = None
+
+# .. setting_name: BUNDLE_ASSET_URL_STORAGE_BUCKET
+# .. setting_default: None
+# .. setting_description: When this is set, `BUNDLE_ASSET_URL_STORAGE_KEY` is
+#  set, and `boto3` is installed, this is used as the bucket name for blockstore assets stored in S3.
+#  Otherwise, the default AWS_STORAGE_BUCKET_NAME is used instead.
+#  See `blockstore.apps.bundles.storage.LongLivedSignedUrlStorage` for details.
+BUNDLE_ASSET_URL_STORAGE_BUCKET = None
+
+# .. setting_name: BUNDLE_ASSET_URL_STORAGE_PREFIX
+# .. setting_default: None
+# .. setting_description: When this is set, `BUNDLE_ASSET_URL_STORAGE_KEY` is
+#  set, and `boto3` is installed, this is used as the path prefix for blockstore assets stored in S3.
+#  Otherwise, the default AWS_LOCATION is used instead.
+#  See `blockstore.apps.bundles.storage.LongLivedSignedUrlStorage` for details.
+BUNDLE_ASSET_URL_STORAGE_PREFIX = None

From 4ac34e3dd5b7c6d7ba27799bc04109ea28af50c9 Mon Sep 17 00:00:00 2001
From: Jillian Vogel <jill@opencraft.com>
Date: Wed, 26 Jan 2022 17:38:42 +1030
Subject: [PATCH 06/14] fix: handle case where no BundleVersion exists

and ensure that the related fields are pre-fetched when using the Python API (performance tweak, based on the REST API querysets.)
---
 blockstore/apps/api/methods.py | 79 ++++++++++++++++++++++++++--------
 1 file changed, 62 insertions(+), 17 deletions(-)

diff --git a/blockstore/apps/api/methods.py b/blockstore/apps/api/methods.py
index c9e224a8..25e33087 100644
--- a/blockstore/apps/api/methods.py
+++ b/blockstore/apps/api/methods.py
@@ -76,7 +76,7 @@ def get_bundles(uuids=None, text_search=None):
     """
     Get the details of all bundles.
     """
-    bundles_queryset = models.Bundle.objects.all()
+    bundles_queryset = _bundle_queryset()
     if uuids:
         bundles_queryset = bundles_queryset.filter(uuid__in=uuids)
     if text_search:
@@ -154,7 +154,7 @@ def get_or_create_bundle_draft(bundle_uuid, draft_name):
     Retrieve metadata about the specified draft, creating a new one if it does not exist yet.
     """
     try:
-        draft_model = models.Draft.objects.get(bundle__uuid=bundle_uuid, name=draft_name)
+        draft_model = _draft_queryset().get(bundle__uuid=bundle_uuid, name=draft_name)
     except models.Draft.DoesNotExist:
         bundle_model = _get_bundle_model(bundle_uuid)
         draft_model = models.Draft(
@@ -201,6 +201,8 @@ def get_bundle_version(bundle_uuid, version_number=None):
     Get the details of the specified bundle version
     """
     bundle_version_model = _get_bundle_version_model(bundle_uuid, version_number)
+    if bundle_version_model is None:
+        return None
     return _bundle_version_data_from_model(bundle_version_model)
 
 
@@ -209,8 +211,12 @@ def get_bundle_version_files(bundle_uuid, version_number):
     Get a list of the files in the specified bundle version
     """
     if version_number == 0:
+        # There are no files in the initial version of a bundle
         return []
-    return get_bundle_version(bundle_uuid, version_number).files.values()
+    bundle_version = get_bundle_version(bundle_uuid, version_number)
+    if bundle_version:
+        return bundle_version.files.values()
+    return []
 
 
 def get_bundle_version_links(bundle_uuid, version_number):
@@ -218,26 +224,33 @@ def get_bundle_version_links(bundle_uuid, version_number):
     Get a dictionary of the links in the specified bundle version
     """
     if version_number == 0:
+        # There are no links in the initial version of a bundle
         return {}
-    return get_bundle_version(bundle_uuid, version_number).links
+    bundle_version = get_bundle_version(bundle_uuid, version_number)
+    if bundle_version:
+        return bundle_version.links
+    return {}
 
 
 def get_bundle_files_dict(bundle_uuid, use_draft=None):
     """
-    Get a dict of all the files in the specified bundle.
+    Get a dict of all the files in the specified bundle or draft.
 
     Returns a dict where the keys are the paths (strings) and the values are
     BundleFileData or DraftFileData tuples.
     """
     if use_draft:
         try:
-            draft_model = models.Draft.objects.get(bundle__uuid=bundle_uuid, name=use_draft)
+            draft_model = _draft_queryset().get(bundle__uuid=bundle_uuid, name=use_draft)
         except models.Draft.DoesNotExist:
             pass
         else:
             return _draft_data_from_model(draft_model).files
 
-    return get_bundle_version(bundle_uuid).files
+    bundle_version = get_bundle_version(bundle_uuid)
+    if bundle_version:
+        return bundle_version.files
+    return {}
 
 
 def get_bundle_files(bundle_uuid, use_draft=None):
@@ -256,13 +269,16 @@ def get_bundle_links(bundle_uuid, use_draft=None):
     """
     if use_draft:
         try:
-            draft_model = models.Draft.objects.get(bundle__uuid=bundle_uuid, name=use_draft)
+            draft_model = _draft_queryset().get(bundle__uuid=bundle_uuid, name=use_draft)
         except models.Draft.DoesNotExist:
             pass
         else:
             return _draft_data_from_model(draft_model).links
 
-    return get_bundle_version(bundle_uuid).links
+    bundle_version = get_bundle_version(bundle_uuid)
+    if bundle_version:
+        return get_bundle_version(bundle_uuid).links
+    return {}
 
 
 def get_bundle_file_metadata(bundle_uuid, path, use_draft=None):
@@ -288,7 +304,7 @@ def get_bundle_file_data(bundle_uuid, path, use_draft=None):
 
     if use_draft:
         try:
-            draft_model = models.Draft.objects.get(bundle__uuid=bundle_uuid, name=use_draft)
+            draft_model = _draft_queryset().get(bundle__uuid=bundle_uuid, name=use_draft)
         except models.Draft.DoesNotExist:
             pass
         else:
@@ -297,7 +313,7 @@ def get_bundle_file_data(bundle_uuid, path, use_draft=None):
             with draft_repo.open(staged_draft, path) as file:
                 return file.read()
 
-    bundle_version_model = _get_bundle_version_model(bundle_uuid)
+    bundle_version_model = _get_bundle_version_model(bundle_uuid, 0)
 
     snapshot_repo = SnapshotRepo()
     snapshot = bundle_version_model.snapshot()
@@ -391,6 +407,15 @@ def _collection_data_from_model(collection_model):
     return CollectionData(uuid=collection_model.uuid, title=collection_model.title)
 
 
+def _bundle_queryset():
+    """
+    Returns the bundle model queryset.
+
+    Prefetch the data needed to create BundleData objects.
+    """
+    return models.Bundle.objects.prefetch_related('drafts', 'versions')
+
+
 def _get_bundle_model(bundle_uuid):
     """
     Get Bundle model from UUID.
@@ -398,7 +423,7 @@ def _get_bundle_model(bundle_uuid):
     Raises BundleNotFound if bundle with UUID does not exist.
     """
     try:
-        bundle_model = models.Bundle.objects.get(uuid=bundle_uuid)
+        bundle_model = _bundle_queryset().get(uuid=bundle_uuid)
     except models.Bundle.DoesNotExist as exc:
         raise BundleNotFound("Bundle {} does not exist.".format(bundle_uuid)) from exc
     return bundle_model
@@ -408,17 +433,28 @@ def _bundle_data_from_model(bundle_model):
     """
     Create and return BundleData from bundle model.
     """
-    latest_bundle_version_model = bundle_model.get_bundle_version()
+    latest_version = bundle_model.versions.order_by('-version_num').first()
+    latest_version_num = latest_version.version_num if latest_version else 0
+
     return BundleData(
         uuid=bundle_model.uuid,
         title=bundle_model.title,
         description=bundle_model.description,
         slug=bundle_model.slug,
         drafts={draft.name: draft.uuid for draft in bundle_model.drafts.all()},
-        latest_version=latest_bundle_version_model.version_num if latest_bundle_version_model else 0,
+        latest_version=latest_version_num,
     )
 
 
+def _draft_queryset():
+    """
+    Returns the draft model queryset.
+
+    Prefetch the data needed to create DraftData objects.
+    """
+    return models.Draft.objects.select_related('bundle')
+
+
 def _get_draft_model(draft_uuid):
     """
     Get Draft model from UUID.
@@ -426,7 +462,7 @@ def _get_draft_model(draft_uuid):
     Raises DraftNotFound if draft with UUID does not exist.
     """
     try:
-        draft_model = models.Draft.objects.get(uuid=draft_uuid)
+        draft_model = _draft_queryset().get(uuid=draft_uuid)
     except models.Draft.DoesNotExist as exc:
         raise DraftNotFound("Draft {} does not exist.".format(draft_uuid)) from exc
     return draft_model
@@ -475,6 +511,15 @@ def _draft_data_from_model(draft_model):
     )
 
 
+def _bundle_version_queryset():
+    """
+    Returns the bundle version model queryset.
+
+    Prefetch the data needed to create BundleVersionData objects.
+    """
+    return models.BundleVersion.objects.select_related('bundle')
+
+
 def _get_bundle_version_model(bundle_uuid, version_number=None):
     """
     Get BundleVersion from bundle UUID and version number.
@@ -487,8 +532,8 @@ def _get_bundle_version_model(bundle_uuid, version_number=None):
     if version_number:
         filter_kwargs['version_num'] = version_number
 
-    bundle_version_model = models.BundleVersion.objects.filter(**filter_kwargs).order_by('-version_num').first()
-    if bundle_version_model is None:
+    bundle_version_model = _bundle_version_queryset().filter(**filter_kwargs).order_by('-version_num').first()
+    if version_number and bundle_version_model is None:
         raise BundleVersionNotFound("Bundle Version {},{} does not exist.".format(bundle_uuid, version_number))
     return bundle_version_model
 

From 4934a3f0912e6f98a555ba23b6e8c6cfb7cff3bd Mon Sep 17 00:00:00 2001
From: Jillian Vogel <jill@opencraft.com>
Date: Wed, 2 Feb 2022 16:22:37 +1030
Subject: [PATCH 07/14] feat: adds settings.BUNDLE_ASSET_STORAGE_SETTINGS

to be consistent with the other custom storage settings in edx-platform.
---
 blockstore/apps/bundles/storage.py            |  5 +---
 blockstore/apps/bundles/tests/test_storage.py |  7 +++++-
 blockstore/settings/base.py                   | 25 +++++++++----------
 3 files changed, 19 insertions(+), 18 deletions(-)

diff --git a/blockstore/apps/bundles/storage.py b/blockstore/apps/bundles/storage.py
index 62870ad0..8d30e5a7 100644
--- a/blockstore/apps/bundles/storage.py
+++ b/blockstore/apps/bundles/storage.py
@@ -97,11 +97,8 @@ def __init__(self):
         s3_backend_args = dict(
             access_key=key,
             secret_key=secret,
+            **settings.BUNDLE_ASSET_STORAGE_SETTINGS['STORAGE_KWARGS']
         )
-        if settings.BUNDLE_ASSET_URL_STORAGE_BUCKET:
-            s3_backend_args['bucket_name'] = settings.BUNDLE_ASSET_URL_STORAGE_BUCKET
-        if settings.BUNDLE_ASSET_URL_STORAGE_PREFIX:
-            s3_backend_args['location'] = settings.BUNDLE_ASSET_URL_STORAGE_PREFIX
 
         self.s3_backend = S3Boto3Storage(**s3_backend_args)
 
diff --git a/blockstore/apps/bundles/tests/test_storage.py b/blockstore/apps/bundles/tests/test_storage.py
index 6bc48f12..7688ac60 100644
--- a/blockstore/apps/bundles/tests/test_storage.py
+++ b/blockstore/apps/bundles/tests/test_storage.py
@@ -31,7 +31,12 @@ def url(self, name):
 )
 _patch_s3_credentials = override_settings(
     BUNDLE_ASSET_URL_STORAGE_KEY="a-key", BUNDLE_ASSET_URL_STORAGE_SECRET="a-secret",
-    BUNDLE_ASSET_URL_STORAGE_BUCKET='example-bucket', BUNDLE_ASSET_URL_STORAGE_PREFIX='s3/',
+    BUNDLE_ASSET_STORAGE_SETTINGS={
+        'STORAGE_KWARGS': {
+            'bucket_name': 'example-bucket',
+            'location': 's3/',
+        },
+    },
 )
 
 
diff --git a/blockstore/settings/base.py b/blockstore/settings/base.py
index 5f4eacc0..61ec36e2 100644
--- a/blockstore/settings/base.py
+++ b/blockstore/settings/base.py
@@ -323,18 +323,17 @@ def root(*x):
 #  See `blockstore.apps.bundles.storage.LongLivedSignedUrlStorage` for details.
 BUNDLE_ASSET_URL_STORAGE_SECRET = None
 
-# .. setting_name: BUNDLE_ASSET_URL_STORAGE_BUCKET
-# .. setting_default: None
+# .. setting_name: BUNDLE_ASSET_STORAGE_SETTINGS
+# .. setting_default: dict, appropriate for file system storage.
 # .. setting_description: When this is set, `BUNDLE_ASSET_URL_STORAGE_KEY` is
-#  set, and `boto3` is installed, this is used as the bucket name for blockstore assets stored in S3.
-#  Otherwise, the default AWS_STORAGE_BUCKET_NAME is used instead.
+#  set, and `boto3` is installed, this provides the bucket name and location for blockstore assets stored in S3.
 #  See `blockstore.apps.bundles.storage.LongLivedSignedUrlStorage` for details.
-BUNDLE_ASSET_URL_STORAGE_BUCKET = None
-
-# .. setting_name: BUNDLE_ASSET_URL_STORAGE_PREFIX
-# .. setting_default: None
-# .. setting_description: When this is set, `BUNDLE_ASSET_URL_STORAGE_KEY` is
-#  set, and `boto3` is installed, this is used as the path prefix for blockstore assets stored in S3.
-#  Otherwise, the default AWS_LOCATION is used instead.
-#  See `blockstore.apps.bundles.storage.LongLivedSignedUrlStorage` for details.
-BUNDLE_ASSET_URL_STORAGE_PREFIX = None
+BUNDLE_ASSET_STORAGE_SETTINGS = dict(
+    # Backend storage
+    # STORAGE_CLASS='storages.backends.s3boto.S3BotoStorage',
+    # STORAGE_KWARGS=dict(bucket='bundle-asset-bucket', location='/path-to-bundles/'),
+    STORAGE_KWARGS=dict(
+        location=MEDIA_ROOT,
+        base_url=MEDIA_URL,
+    ),
+)

From 25b09e129bd1e65272d0bdc210cbbc22741cfee0 Mon Sep 17 00:00:00 2001
From: Jillian Vogel <jill@opencraft.com>
Date: Wed, 2 Feb 2022 16:23:22 +1030
Subject: [PATCH 08/14] fix: use pytest instead of manage.py test

Several test files were being skipped (e.g. test_storage.py). Using
pytest instead restores our test coverage to 96%.
---
 Makefile                                         | 2 +-
 blockstore/apps/api/tests/test_blockstore_api.py | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index a6b3c06e..8bc9691a 100644
--- a/Makefile
+++ b/Makefile
@@ -85,7 +85,7 @@ static:  ## Collect static files
 	${VENV_BIN}/python manage.py collectstatic --noinput
 
 test: clean ## Run tests and generate coverage report
-	${VENV_BIN}/coverage run ./manage.py test blockstore --settings=blockstore.settings.test
+	${VENV_BIN}/coverage run ${VENV_BIN}/pytest blockstore --ds=blockstore.settings.test
 	${VENV_BIN}/coverage html
 	${VENV_BIN}/coverage xml
 	${VENV_BIN}/diff-cover coverage.xml --html-report diff-cover.html --compare-branch=origin/master
diff --git a/blockstore/apps/api/tests/test_blockstore_api.py b/blockstore/apps/api/tests/test_blockstore_api.py
index a7c3a962..dd34bcdd 100644
--- a/blockstore/apps/api/tests/test_blockstore_api.py
+++ b/blockstore/apps/api/tests/test_blockstore_api.py
@@ -13,6 +13,7 @@
 BAD_UUID = UUID('12345678-0000-0000-0000-000000000000')
 
 
+@pytest.mark.django_db
 class BlockstoreApiClientTest(unittest.TestCase):
     """
     Test for the Blockstore API Client.

From c816518823a31d0aee066a8225acbb03e41b3527 Mon Sep 17 00:00:00 2001
From: Jillian Vogel <jill@opencraft.com>
Date: Wed, 2 Feb 2022 17:22:42 +1030
Subject: [PATCH 09/14] refactor: re-adds force_browser_url to python API

Since the blockstore python API is run from within the LMS/Studio, we
need to replace their container names in the devstack when returning
URLs to the browser.

Also adds tests for this method, and consolidates some other code to
improve test coverage. Currently at 98%.
---
 blockstore/apps/api/__init__.py               |  2 ++
 blockstore/apps/api/methods.py                | 36 ++++++++++++-------
 .../apps/api/tests/test_blockstore_api.py     | 23 ++++++++++--
 3 files changed, 47 insertions(+), 14 deletions(-)

diff --git a/blockstore/apps/api/__init__.py b/blockstore/apps/api/__init__.py
index a78bde7c..11a1481a 100644
--- a/blockstore/apps/api/__init__.py
+++ b/blockstore/apps/api/__init__.py
@@ -45,6 +45,8 @@
     # Links:
     get_bundle_links,
     get_bundle_version_links,
+    # Misc:
+    force_browser_url,
 )
 from .exceptions import (
     BlockstoreException,
diff --git a/blockstore/apps/api/methods.py b/blockstore/apps/api/methods.py
index 25e33087..1e2a9cc5 100644
--- a/blockstore/apps/api/methods.py
+++ b/blockstore/apps/api/methods.py
@@ -3,6 +3,7 @@
 """
 
 import base64
+import re
 import crum
 
 from django.db.models import Q
@@ -214,9 +215,7 @@ def get_bundle_version_files(bundle_uuid, version_number):
         # There are no files in the initial version of a bundle
         return []
     bundle_version = get_bundle_version(bundle_uuid, version_number)
-    if bundle_version:
-        return bundle_version.files.values()
-    return []
+    return list(bundle_version.files.values() if bundle_version else [])
 
 
 def get_bundle_version_links(bundle_uuid, version_number):
@@ -227,9 +226,7 @@ def get_bundle_version_links(bundle_uuid, version_number):
         # There are no links in the initial version of a bundle
         return {}
     bundle_version = get_bundle_version(bundle_uuid, version_number)
-    if bundle_version:
-        return bundle_version.links
-    return {}
+    return bundle_version.links if bundle_version else {}
 
 
 def get_bundle_files_dict(bundle_uuid, use_draft=None):
@@ -248,9 +245,7 @@ def get_bundle_files_dict(bundle_uuid, use_draft=None):
             return _draft_data_from_model(draft_model).files
 
     bundle_version = get_bundle_version(bundle_uuid)
-    if bundle_version:
-        return bundle_version.files
-    return {}
+    return bundle_version.files if bundle_version else {}
 
 
 def get_bundle_files(bundle_uuid, use_draft=None):
@@ -276,9 +271,7 @@ def get_bundle_links(bundle_uuid, use_draft=None):
             return _draft_data_from_model(draft_model).links
 
     bundle_version = get_bundle_version(bundle_uuid)
-    if bundle_version:
-        return get_bundle_version(bundle_uuid).links
-    return {}
+    return get_bundle_version(bundle_uuid).links if bundle_version else {}
 
 
 def get_bundle_file_metadata(bundle_uuid, path, use_draft=None):
@@ -376,6 +369,25 @@ def set_draft_link(draft_uuid, link_name, bundle_uuid, version):
         raise serializers.ValidationError("Link cycle detected: Cannot create draft.") from exc
 
 
+REGEX_BROWSER_URL = re.compile(r'http://edx.devstack.(studio|lms):')
+
+
+def force_browser_url(blockstore_file_url):
+    """
+    Ensure that the given devstack URL is a URL accessible from the end user's browser.
+    """
+    # Hack: on some devstacks, we must necessarily use different URLs for
+    # accessing Blockstore file data from within and outside of docker
+    # containers, but Blockstore has no way of knowing which case any particular
+    # request is for. So it always returns a URL suitable for use from within
+    # the container. Only this edxapp can transform the URL at the last second,
+    # knowing that in this case it's going to the user's browser and not being
+    # read by edxapp.
+    # In production, the same S3 URLs get used for internal and external access
+    # so this hack is not necessary.
+    return re.sub(REGEX_BROWSER_URL, 'http://localhost:', blockstore_file_url)
+
+
 def _encode_str_for_draft(input_str):
     """
     Given a string, return UTF-8 representation that is then base64 encoded.
diff --git a/blockstore/apps/api/tests/test_blockstore_api.py b/blockstore/apps/api/tests/test_blockstore_api.py
index dd34bcdd..a9c3de20 100644
--- a/blockstore/apps/api/tests/test_blockstore_api.py
+++ b/blockstore/apps/api/tests/test_blockstore_api.py
@@ -64,6 +64,16 @@ def test_nonexistent_bundle(self):
         with pytest.raises(api.BundleNotFound):
             api.get_bundle(BAD_UUID)
 
+    def test_nonexistent_bundle_version(self):
+        """
+        Request a bundle version that doesn't exist.
+        """
+        # If you don't pass a version number, then it just returns None.
+        assert api.get_bundle_version(BAD_UUID) is None
+        # But if you do want a specific version, it 404s
+        with pytest.raises(api.BundleVersionNotFound):
+            api.get_bundle_version(BAD_UUID, '1')
+
     def test_bundle_crud(self):
         """ Create, Fetch, Update, and Delete a Bundle """
         coll = api.create_collection("Test Collection")
@@ -213,7 +223,7 @@ def test_bundle_version(self):
         )
 
         no_files = api.get_bundle_version_files(bundle.uuid, 0)
-        assert no_files == []
+        assert not no_files
 
         # Create and commit a draft
         draft = api.get_or_create_bundle_draft(bundle.uuid, draft_name="test-draft")
@@ -260,7 +270,7 @@ def test_links(self):
         lib2_draft = api.get_or_create_bundle_draft(lib2_bundle.uuid, draft_name="other-draft")
         course_bundle = api.create_bundle(coll.uuid, title="Library 1", slug="course")
         course_draft = api.get_or_create_bundle_draft(course_bundle.uuid, draft_name="test-draft")
-        assert api.get_bundle_version_links(course_bundle.uuid, 0) == {}
+        assert not api.get_bundle_version_links(course_bundle.uuid, 0)
 
         # To create links, we need valid BundleVersions, which requires having committed at least one change:
         api.write_draft_file(lib1_draft.uuid, "lib1-data.txt", "hello world")
@@ -307,3 +317,12 @@ def test_links(self):
         api.delete_draft(course_draft.uuid)
         course_links2 = api.get_bundle_links(course_bundle.uuid, use_draft=course_draft.name)
         assert course_links == course_links2
+
+    def test_force_browser_url(self):
+        """
+        Test the browser URL hack for devstacks.
+        """
+        assert api.force_browser_url('http://edx.devstack.studio:18010/media/snapshot/definition.xml') ==\
+            'http://localhost:18010/media/snapshot/definition.xml'
+        assert api.force_browser_url('http://edx.devstack.lms:18000/media/snapshot/definition.xml') ==\
+            'http://localhost:18000/media/snapshot/definition.xml'

From 949721d5a278570ae48c450d1011cac1c8bc22bd Mon Sep 17 00:00:00 2001
From: Jillian Vogel <jill@opencraft.com>
Date: Mon, 7 Feb 2022 15:15:42 +1030
Subject: [PATCH 10/14] fix: allow get_current_request to be mocked in tests

---
 blockstore/apps/api/methods.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/blockstore/apps/api/methods.py b/blockstore/apps/api/methods.py
index 1e2a9cc5..616aa1da 100644
--- a/blockstore/apps/api/methods.py
+++ b/blockstore/apps/api/methods.py
@@ -4,7 +4,7 @@
 
 import base64
 import re
-import crum
+from crum import get_current_request
 
 from django.db.models import Q
 from rest_framework import serializers
@@ -484,7 +484,7 @@ def _build_absolute_uri(url):
     """
     Build an absolute URI from the given url, using the CRUM middleware's stored request.
     """
-    request = crum.get_current_request()
+    request = get_current_request()
     return request.build_absolute_uri(url)
 
 

From 02fa2488baf65d201c4a9c0e563983da19b8a4ff Mon Sep 17 00:00:00 2001
From: Jillian Vogel <jill@opencraft.com>
Date: Tue, 15 Feb 2022 15:30:39 +1030
Subject: [PATCH 11/14] fix: allow blockstore storage to be configured
 separately from default storage

---
 blockstore/apps/bundles/storage.py            | 48 +++++++++------
 blockstore/apps/bundles/tests/test_storage.py | 61 +++++++++++++++++--
 blockstore/settings/base.py                   | 18 ++----
 3 files changed, 90 insertions(+), 37 deletions(-)

diff --git a/blockstore/apps/bundles/storage.py b/blockstore/apps/bundles/storage.py
index 8d30e5a7..9b0a453b 100644
--- a/blockstore/apps/bundles/storage.py
+++ b/blockstore/apps/bundles/storage.py
@@ -91,15 +91,17 @@ def __init__(self):
         if not (key and secret):
             raise self.BackendNotAvailable
 
-        # Provide the bucket name and prefix if configured;
+        # Merge the special key and secret with extra storage args when configuring
+        # the S3 URLs backend.
         # All other S3 settings will be pulled in automatically from Django settings
         # (such as AWS_QUERYSTRING_EXPIRE and AWS_STORAGE_BUCKET_NAME).
         s3_backend_args = dict(
+            **settings.BUNDLE_ASSET_STORAGE_SETTINGS['STORAGE_KWARGS']
+        )
+        s3_backend_args.update(
             access_key=key,
             secret_key=secret,
-            **settings.BUNDLE_ASSET_STORAGE_SETTINGS['STORAGE_KWARGS']
         )
-
         self.s3_backend = S3Boto3Storage(**s3_backend_args)
 
     def url(self, name):
@@ -122,55 +124,65 @@ def __init__(self):
         """
         Initialize an instance of AssetStorage.
 
+        Use the BUNDLE_ASSET_STORAGE_SETTINGS['STORAGE_CLASS'] if provided; otherwise
+        fall back to the default storage class.
+
         If `LongLivedSignedUrlStorage` is active, then instantiate an instance of
-        it for generating URLs; otherwise, fall back to the default storage class.
+        it for generating URLs; otherwise, use the asset storage class defined above.
         """
+        storage_class = settings.BUNDLE_ASSET_STORAGE_SETTINGS.get('STORAGE_CLASS')
+        storage_kwargs = settings.BUNDLE_ASSET_STORAGE_SETTINGS.get('STORAGE_KWARGS', {})
+        if storage_class:
+            self.asset_backend = get_storage_class(storage_class)(**storage_kwargs)
+        else:
+            self.asset_backend = default_storage
+
         try:
             self.url_backend = LongLivedSignedUrlStorage()
         except LongLivedSignedUrlStorage.BackendNotAvailable:
-            self.url_backend = default_storage
+            self.url_backend = self.asset_backend
 
     def url(self, name):
         return self.url_backend.url(name)
 
     def delete(self, name):
-        return default_storage.delete(name)
+        return self.asset_backend.delete(name)
 
     def exists(self, name):
-        return default_storage.exists(name)
+        return self.asset_backend.exists(name)
 
     def listdir(self, path):
-        return default_storage.listdir(path)
+        return self.asset_backend.listdir(path)
 
     def path(self, name):
-        return default_storage.path(name)
+        return self.asset_backend.path(name)
 
     def size(self, name):
-        return default_storage.size(name)
+        return self.asset_backend.size(name)
 
     def get_accessed_time(self, name):
-        return default_storage.get_accessed_time(name)
+        return self.asset_backend.get_accessed_time(name)
 
     def get_created_time(self, name):
-        return default_storage.get_created_time(name)
+        return self.asset_backend.get_created_time(name)
 
     def get_modified_time(self, name):
-        return default_storage.get_modified_time(name)
+        return self.asset_backend.get_modified_time(name)
 
     def get_valid_name(self, name):
-        return default_storage.get_valid_name(name)
+        return self.asset_backend.get_valid_name(name)
 
     def get_alternative_name(self, file_root, file_ext):
-        return default_storage.get_alternative_name(file_root, file_ext)
+        return self.asset_backend.get_alternative_name(file_root, file_ext)
 
     def get_available_name(self, name, max_length=None):
-        return default_storage.get_available_name(name, max_length=None)
+        return self.asset_backend.get_available_name(name, max_length=None)
 
     def _open(self, name, mode='rb'):
-        return default_storage._open(name, mode=mode)  # pylint: disable=protected-access
+        return self.asset_backend._open(name, mode=mode)  # pylint: disable=protected-access
 
     def _save(self, name, content):
-        return default_storage._save(name, content)  # pylint: disable=protected-access
+        return self.asset_backend._save(name, content)  # pylint: disable=protected-access
 
 
 default_asset_storage = AssetStorage()
diff --git a/blockstore/apps/bundles/tests/test_storage.py b/blockstore/apps/bundles/tests/test_storage.py
index 7688ac60..5c49756e 100644
--- a/blockstore/apps/bundles/tests/test_storage.py
+++ b/blockstore/apps/bundles/tests/test_storage.py
@@ -1,7 +1,7 @@
 """
 Tests for storage classes in storage.py
 """
-from unittest.mock import patch
+from unittest.mock import patch, MagicMock
 
 from django.test import override_settings
 import pytest
@@ -23,18 +23,42 @@ def url(self, name):
         return f"https://{self.bucket_name}/{self.location}{name}"
 
 
+def get_storage_class(class_name):
+    """
+    Returns the MockS3Backend if S3Boto3Storage requested,
+    or a mock class spec'd on default_storage for any other class name.
+    """
+    if class_name == 'storages.backends.s3boto3.S3Boto3Storage':
+        return _MockS3backend
+    return MagicMock(spec=storage_module.default_storage).__class__
+
+
 _patch_default_storage = patch.object(
     storage_module, 'default_storage', autospec=True
 )
 _patch_get_storage_class = patch.object(
-    storage_module, 'get_storage_class', autospec=True, return_value=_MockS3backend
+    storage_module, 'get_storage_class', autospec=True, side_effect=get_storage_class
 )
-_patch_s3_credentials = override_settings(
+_patch_s3_long_lived_credentials = override_settings(
     BUNDLE_ASSET_URL_STORAGE_KEY="a-key", BUNDLE_ASSET_URL_STORAGE_SECRET="a-secret",
     BUNDLE_ASSET_STORAGE_SETTINGS={
+        'STORAGE_CLASS': 'storages.backends.some.other.backend',
+        'STORAGE_KWARGS': {
+            'bucket_name': 'example-bucket',
+            'location': 's3/',
+            'access_key': 'another_key',
+            'secret_key': 'another_secret',
+        },
+    },
+)
+_patch_s3_credentials = override_settings(
+    BUNDLE_ASSET_STORAGE_SETTINGS={
+        'STORAGE_CLASS': 'storages.backends.some.other.backend',
         'STORAGE_KWARGS': {
             'bucket_name': 'example-bucket',
             'location': 's3/',
+            'access_key': 'another_key',
+            'secret_key': 'another_secret',
         },
     },
 )
@@ -56,7 +80,7 @@ def test_asset_storage_long_lived_urls_disabled(mock_default_storage):
     mock_default_storage.get_accessed_time.assert_called_once_with('xyz')
 
 
-@_patch_s3_credentials
+@_patch_s3_long_lived_credentials
 @_patch_get_storage_class
 @_patch_default_storage
 def test_asset_storage_long_lived_urls_enabled(mock_default_storage, *_args):
@@ -70,12 +94,37 @@ def test_asset_storage_long_lived_urls_enabled(mock_default_storage, *_args):
     assert backend.url_backend.s3_backend.secret_key == "a-secret"
     assert backend.url_backend.s3_backend.bucket_name == "example-bucket"
     assert backend.url_backend.s3_backend.location == "s3/"
+    assert backend.asset_backend.access_key == "another_key"
+    assert backend.asset_backend.secret_key == "another_secret"
+    assert backend.asset_backend.bucket_name == "example-bucket"
+    assert backend.asset_backend.location == "s3/"
     assert backend.url('abc') == "https://example-bucket/s3/abc"
     backend.listdir('123')
     backend.get_accessed_time('xyz')
     assert not mock_default_storage.url.called
-    mock_default_storage.listdir.assert_called_once_with('123')
-    mock_default_storage.get_accessed_time.assert_called_once_with('xyz')
+    backend.asset_backend.listdir.assert_called_once_with('123')
+    backend.asset_backend.get_accessed_time.assert_called_once_with('xyz')
+
+
+@_patch_s3_credentials
+@_patch_get_storage_class
+@_patch_default_storage
+def test_asset_storage_basic_s3(mock_default_storage, *_args):
+    """
+    Test that `AssetStorage` is configured as expected when there's no long-lived URL signing credentials configured.
+    """
+    backend = storage_module.AssetStorage()
+    assert backend.url_backend is backend.asset_backend
+    assert backend.asset_backend.access_key == "another_key"
+    assert backend.asset_backend.secret_key == "another_secret"
+    assert backend.asset_backend.bucket_name == "example-bucket"
+    assert backend.asset_backend.location == "s3/"
+    backend.url('abc')
+    backend.listdir('123')
+    backend.get_accessed_time('xyz')
+    assert not mock_default_storage.url.called
+    backend.asset_backend.listdir.assert_called_once_with('123')
+    backend.asset_backend.get_accessed_time.assert_called_once_with('xyz')
 
 
 @_patch_default_storage
diff --git a/blockstore/settings/base.py b/blockstore/settings/base.py
index 61ec36e2..ec8856fb 100644
--- a/blockstore/settings/base.py
+++ b/blockstore/settings/base.py
@@ -324,16 +324,8 @@ def root(*x):
 BUNDLE_ASSET_URL_STORAGE_SECRET = None
 
 # .. setting_name: BUNDLE_ASSET_STORAGE_SETTINGS
-# .. setting_default: dict, appropriate for file system storage.
-# .. setting_description: When this is set, `BUNDLE_ASSET_URL_STORAGE_KEY` is
-#  set, and `boto3` is installed, this provides the bucket name and location for blockstore assets stored in S3.
-#  See `blockstore.apps.bundles.storage.LongLivedSignedUrlStorage` for details.
-BUNDLE_ASSET_STORAGE_SETTINGS = dict(
-    # Backend storage
-    # STORAGE_CLASS='storages.backends.s3boto.S3BotoStorage',
-    # STORAGE_KWARGS=dict(bucket='bundle-asset-bucket', location='/path-to-bundles/'),
-    STORAGE_KWARGS=dict(
-        location=MEDIA_ROOT,
-        base_url=MEDIA_URL,
-    ),
-)
+# .. setting_default: empty dict, uses django DEFAULT_STORAGE_CLASS and settings.
+# .. setting_description: Provide `STORAGE_CLASS` and (optional) `STORAGE_KWARGS`
+#  to configure the storage settings for bundle asset files.
+#  See `blockstore.apps.bundles.storage.AssetStorage` for details.
+BUNDLE_ASSET_STORAGE_SETTINGS = {}

From 83275300903268f0f0851caf1351877cb2cb31f3 Mon Sep 17 00:00:00 2001
From: Jillian Vogel <jill@opencraft.com>
Date: Tue, 15 Feb 2022 17:23:06 +1030
Subject: [PATCH 12/14] docs: updates test docs

---
 blockstore/apps/api/tests/test_blockstore_api.py | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/blockstore/apps/api/tests/test_blockstore_api.py b/blockstore/apps/api/tests/test_blockstore_api.py
index a9c3de20..b6d98ace 100644
--- a/blockstore/apps/api/tests/test_blockstore_api.py
+++ b/blockstore/apps/api/tests/test_blockstore_api.py
@@ -17,10 +17,6 @@
 class BlockstoreApiClientTest(unittest.TestCase):
     """
     Test for the Blockstore API Client.
-
-    The goal of these tests is not to test that Blockstore works correctly, but
-    that the API client can interact with it and all the API client methods
-    work.
     """
     def setUp(self):
         super().setUp()

From 59787c8826842f5afd98a29e72989adcbd3cda54 Mon Sep 17 00:00:00 2001
From: Jillian Vogel <jill@opencraft.com>
Date: Thu, 17 Feb 2022 13:41:51 +1030
Subject: [PATCH 13/14] fix: using the data models in the open edx
 blockstore_api

so minor changes were required
---
 blockstore/apps/api/data.py      | 18 ++++++------------
 blockstore/apps/bundles/links.py |  8 +++++++-
 2 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/blockstore/apps/api/data.py b/blockstore/apps/api/data.py
index 96b85110..f5cbe9c9 100644
--- a/blockstore/apps/api/data.py
+++ b/blockstore/apps/api/data.py
@@ -7,13 +7,7 @@
 
 import attr
 
-from blockstore.apps.bundles.links import Dependency
-
-
-def _convert_to_uuid(value):
-    if not isinstance(value, UUID):
-        return UUID(value)
-    return value
+from blockstore.apps.bundles.links import convert_to_uuid, Dependency
 
 
 @attr.s(frozen=True)
@@ -21,7 +15,7 @@ class CollectionData:
     """
     Metadata about a blockstore collection
     """
-    uuid = attr.ib(type=UUID, converter=_convert_to_uuid)
+    uuid = attr.ib(type=UUID, converter=convert_to_uuid)
     title = attr.ib(type=str)
 
 
@@ -30,7 +24,7 @@ class BundleData:
     """
     Metadata about a blockstore bundle
     """
-    uuid = attr.ib(type=UUID, converter=_convert_to_uuid)
+    uuid = attr.ib(type=UUID, converter=convert_to_uuid)
     title = attr.ib(type=str)
     description = attr.ib(type=str)
     slug = attr.ib(type=str)
@@ -44,8 +38,8 @@ class DraftData:
     """
     Metadata about a blockstore draft
     """
-    uuid = attr.ib(type=UUID, converter=_convert_to_uuid)
-    bundle_uuid = attr.ib(type=UUID, converter=_convert_to_uuid)
+    uuid = attr.ib(type=UUID, converter=convert_to_uuid)
+    bundle_uuid = attr.ib(type=UUID, converter=convert_to_uuid)
     name = attr.ib(type=str)
     created_at = attr.ib(type=datetime, validator=attr.validators.instance_of(datetime))
     updated_at = attr.ib(type=datetime, validator=attr.validators.instance_of(datetime))
@@ -58,7 +52,7 @@ class BundleVersionData:
     """
     Metadata about a blockstore bundle version.
     """
-    bundle_uuid = attr.ib(type=UUID, converter=_convert_to_uuid)
+    bundle_uuid = attr.ib(type=UUID, converter=convert_to_uuid)
     version = attr.ib(type=int, validator=attr.validators.instance_of(int))
     change_description = attr.ib(type=str)
     created_at = attr.ib(type=datetime, validator=attr.validators.instance_of(datetime))
diff --git a/blockstore/apps/bundles/links.py b/blockstore/apps/bundles/links.py
index c70b12cf..d21218e7 100644
--- a/blockstore/apps/bundles/links.py
+++ b/blockstore/apps/bundles/links.py
@@ -11,12 +11,18 @@
 import attr
 
 
+def convert_to_uuid(value):
+    if not isinstance(value, UUID):
+        return UUID(value)
+    return value
+
+
 @attr.s(frozen=True)
 class Dependency:
     """
     A Dependency is a pointer to exactly one Bundle + Version + Snapshot.
     """
-    bundle_uuid = attr.ib(type=UUID)
+    bundle_uuid = attr.ib(type=UUID, converter=convert_to_uuid)
     version = attr.ib(type=int)
     snapshot_digest = attr.ib(type=bytes)
 

From 054ab812bfcbcb3613b4786d1a70b37fd3d56414 Mon Sep 17 00:00:00 2001
From: Jillian Vogel <jill@opencraft.com>
Date: Mon, 21 Feb 2022 11:14:42 +1030
Subject: [PATCH 14/14] docs: addresses PR review for now-public method

---
 blockstore/apps/bundles/links.py | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/blockstore/apps/bundles/links.py b/blockstore/apps/bundles/links.py
index d21218e7..4d5e3a1e 100644
--- a/blockstore/apps/bundles/links.py
+++ b/blockstore/apps/bundles/links.py
@@ -12,9 +12,12 @@
 
 
 def convert_to_uuid(value):
-    if not isinstance(value, UUID):
-        return UUID(value)
-    return value
+    """
+    Returns a UUID from the given (string or UUID) value.
+    """
+    if isinstance(value, UUID):
+        return value
+    return UUID(value)
 
 
 @attr.s(frozen=True)