Render attachments with shortcodes rather than slugs

The attachments should now be rendered using `{attachment slug}` instead
of `@[slug]`. The `link` attribute can be specified in the shortcode
(for attachments that support it), rather than in the attachment itself.

The attachment subdocument is now reduced to `{oid: File ObjectID}`, and
nodes without attachments should NOT have an `attachment` property at
all (previously it would be an empty dict). This makes querying for
nodes with/out attachments easier.

The CLI command `upgrade_attachment_schema` can do dry-run and remove
empty attachments:

- Added --go to actually perform the database changes.
- Remove empty attachments, so that a node either has one or more
  attachments or no attachments sub-document at all.

The CLI command `upgrade_attachment_usage` converts `@[slug]` to
`{attachment slug}`. It also takes into account 'link' and 'link_custom'
fields on the attachment. After conversion those fields are removed from
the attachment itself.

Simplified maintentance CLI commands that iterate over all projects:
I've moved the common approach (either run on one project or all of
them, skipping deleted ones, giving a message upon dry-run, and showing
duration of the command) to a new _db_projects() function. The new
function is now used by two recently-touched CLI commands; more of them
could be migrated to use this.

pillar/api/projects/utils.py

@@ -1,4 +1,5 @@
 import logging
+import typing
 
 from bson import ObjectId
 from werkzeug import exceptions as wz_exceptions
@@ -135,6 +136,14 @@ def get_node_type(project, node_type_name):
                  if nt['name'] == node_type_name), None)
 
 
+def node_type_dict(project: dict) -> typing.Dict[str, dict]:
+    """Return the node types of the project as dictionary.
+
+    The returned dictionary will be keyed by the node type name.
+    """
+    return {nt['name']: nt for nt in project['node_types']}
+
+
 def project_id(project_url: str) -> ObjectId:
     """Returns the object ID, or raises a ValueError when not found."""
 

pillar/cli/maintenance.py

@@ -31,6 +31,21 @@ manager_maintenance = Manager(
     current_app, usage="Maintenance scripts, to update user groups")
 
 
+def _single_logger(*args, level=logging.INFO, **kwargs):
+    """Construct a logger function that's only logging once."""
+
+    shown = False
+
+    def log_once():
+        nonlocal shown
+        if shown:
+            return
+        log.log(level, *args, **kwargs)
+        shown = True
+
+    return log_once
+
+
 @manager_maintenance.command
 def find_duplicate_users():
     """Finds users that have the same BlenderID user_id."""
@@ -471,24 +486,15 @@ def replace_pillar_node_type_schemas(project_url=None, all_projects=False, missi
     Non-standard node types are left alone.
     """
 
-    if sum([bool(project_url), all_projects, bool(project_id)]) != 1:
-        log.error('Use either --project, --id, or --all.')
-        return 1
-
     from pillar.api.utils.authentication import force_cli_user
     force_cli_user()
 
     from pillar.api.node_types import PILLAR_NAMED_NODE_TYPES
     from pillar.api.utils import remove_private_keys, doc_diff
 
-    projects_collection = current_app.db()['projects']
     will_would = 'Will' if go else 'Would'
-
     projects_changed = projects_seen = 0
-
+    for proj in _db_projects(project_url, all_projects, project_id, go=go):
-    def handle_project(proj):
-        nonlocal projects_changed, projects_seen
-
         projects_seen += 1
 
         orig_proj = copy.deepcopy(proj)
@@ -548,27 +554,9 @@ def replace_pillar_node_type_schemas(project_url=None, all_projects=False, missi
                 raise SystemExit('Error storing project, see log.')
             log.debug('Project saved succesfully.')
 
-    if not go:
-        log.info('Not changing anything, use --go to actually go and change things.')
-
-    if all_projects:
-        for project in projects_collection.find({'_deleted': {'$ne': True}}):
-            handle_project(project)
     log.info('%s %d of %d projects',
              'Changed' if go else 'Would change',
              projects_changed, projects_seen)
-        return
-
-    if project_url:
-        project = projects_collection.find_one({'url': project_url})
-    else:
-        project = projects_collection.find_one({'_id': bson.ObjectId(project_id)})
-
-    if not project:
-        log.error('Project url=%s id=%s not found', project_url, project_id)
-        return 3
-
-    handle_project(project)
 
 
 @manager_maintenance.command
@@ -615,17 +603,17 @@ def remarkdown_comments():
     log.info('errors   : %i', errors)
 
 
-@manager_maintenance.command
 @manager_maintenance.option('-p', '--project', dest='proj_url', nargs='?',
                             help='Project URL')
 @manager_maintenance.option('-a', '--all', dest='all_projects', action='store_true', default=False,
                             help='Replace on all projects.')
-def upgrade_attachment_schema(proj_url=None, all_projects=False):
+@manager_maintenance.option('-g', '--go', dest='go', action='store_true', default=False,
+                            help='Actually perform the changes (otherwise just show as dry-run).')
+def upgrade_attachment_schema(proj_url=None, all_projects=False, go=False):
     """Replaces the project's attachments with the new schema.
 
     Updates both the schema definition and the nodes with attachments (asset, page, post).
     """
-
     if bool(proj_url) == all_projects:
         log.error('Use either --project or --all.')
         return 1
@@ -637,28 +625,29 @@ def upgrade_attachment_schema(proj_url=None, all_projects=False):
     from pillar.api.node_types.page import node_type_page
     from pillar.api.node_types.post import node_type_post
     from pillar.api.node_types import attachments_embedded_schema
-    from pillar.api.utils import remove_private_keys
+    from pillar.api.utils import remove_private_keys, doc_diff
 
     # Node types that support attachments
     node_types = (node_type_asset, node_type_page, node_type_post)
     nts_by_name = {nt['name']: nt for nt in node_types}
 
-    db = current_app.db()
+    nodes_coll = current_app.db('nodes')
-    projects_coll = db['projects']
-    nodes_coll = db['nodes']
-
-    def handle_project(project):
-        log.info('Handling project %s', project['url'])
-
-        replace_schemas(project)
-        replace_attachments(project)
 
     def replace_schemas(project):
+        log_proj = _single_logger('Upgrading schema project %s (%s)',
+                                  project['url'], project['_id'])
+
+        orig_proj = copy.deepcopy(project)
         for proj_nt in project['node_types']:
             nt_name = proj_nt['name']
             if nt_name not in nts_by_name:
                 continue
 
+            if proj_nt['dyn_schema']['attachments'] == attachments_embedded_schema:
+                # Schema already up to date.
+                continue
+
+            log_proj()
             log.info('   - replacing attachment schema on node type "%s"', nt_name)
             pillar_nt = nts_by_name[nt_name]
             proj_nt['dyn_schema']['attachments'] = copy.deepcopy(attachments_embedded_schema)
@@ -671,16 +660,44 @@ def upgrade_attachment_schema(proj_url=None, all_projects=False):
             else:
                 proj_nt['form_schema']['attachments'] = pillar_form_schema
 
+        seen_changes = False
+        for key, val1, val2 in doc_diff(orig_proj, project):
+            if not seen_changes:
+                log.info('Schema changes to project %s (%s):', project['url'], project['_id'])
+                seen_changes = True
+            log.info('    - %30s: %s → %s', key, val1, val2)
+
+        if go:
             # Use Eve to PUT, so we have schema checking.
             db_proj = remove_private_keys(project)
             r, _, _, status = current_app.put_internal('projects', db_proj, _id=project['_id'])
             if status != 200:
                 log.error('Error %i storing altered project %s %s', status, project['_id'], r)
                 raise SystemExit('Error storing project, see log.')
-        log.info('Project saved succesfully.')
+            log.debug('Project saved succesfully.')
 
     def replace_attachments(project):
-        log.info('Upgrading nodes for project %s', project['url'])
+        log_proj = _single_logger('Upgrading nodes for project %s (%s)',
+                                  project['url'], project['_id'])
+
+        # Remove empty attachments
+        if go:
+            res = nodes_coll.update_many(
+                {'properties.attachments': {},
+                 'project': project['_id']},
+                {'$unset': {'properties.attachments': 1}},
+            )
+            if res.matched_count > 0:
+                log_proj()
+                log.info('Removed %d empty attachment dicts', res.modified_count)
+        else:
+            to_remove = nodes_coll.count({'properties.attachments': {},
+                                          'project': project['_id']})
+            if to_remove:
+                log_proj()
+                log.info('Would remove %d empty attachment dicts', to_remove)
+
+        # Convert attachments.
         nodes = nodes_coll.find({
             '_deleted': False,
             'project': project['_id'],
@@ -689,10 +706,22 @@ def upgrade_attachment_schema(proj_url=None, all_projects=False):
         })
         for node in nodes:
             attachments = node['properties']['attachments']
+            if not attachments:
+                # If we're not modifying the database (e.g. go=False),
+                # any attachments={} will not be filtered out earlier.
+                if go or attachments != {}:
+                    log_proj()
+                    log.info('    - Node %s (%s) still has empty attachments %r',
+                             node['_id'], node.get('name'), attachments)
+                continue
+
             if isinstance(attachments, dict):
                 # This node has already been upgraded.
                 continue
 
+            # Upgrade from list [{'slug': 'xxx', 'oid': 'yyy'}, ...]
+            # to dict {'xxx': {'oid': 'yyy'}, ...}
+            log_proj()
             log.info('    - Updating schema on node %s (%s)', node['_id'], node.get('name'))
             new_atts = {}
             for field_info in attachments:
@@ -700,7 +729,9 @@ def upgrade_attachment_schema(proj_url=None, all_projects=False):
                     new_atts[attachment['slug']] = {'oid': attachment['file']}
 
             node['properties']['attachments'] = new_atts
+            log.info('      from %s to %s', attachments, new_atts)
 
+            if go:
                 # Use Eve to PUT, so we have schema checking.
                 db_node = remove_private_keys(node)
                 r, _, _, status = current_app.put_internal('nodes', db_node, _id=node['_id'])
@@ -708,17 +739,177 @@ def upgrade_attachment_schema(proj_url=None, all_projects=False):
                     log.error('Error %i storing altered node %s %s', status, node['_id'], r)
                     raise SystemExit('Error storing node; see log.')
 
+    for proj in _db_projects(proj_url, all_projects, go=go):
+        replace_schemas(proj)
+        replace_attachments(proj)
+
+
+def iter_markdown(proj_node_types: dict, some_node: dict, callback: typing.Callable[[str], str]):
+    """Calls the callback for each MarkDown value in the node.
+
+    Replaces the value in-place with the return value of the callback.
+    """
+    from collections import deque
+    from pillar.api.eve_settings import nodes_schema
+
+    my_log = log.getChild('iter_markdown')
+
+    # Inspect the node type to find properties containing Markdown.
+    node_type_name = some_node['node_type']
+    try:
+        node_type = proj_node_types[node_type_name]
+    except KeyError:
+        raise KeyError(f'Project has no node type {node_type_name}')
+
+    to_visit = deque([
+        (some_node, nodes_schema),
+        (some_node['properties'], node_type['dyn_schema'])])
+    while to_visit:
+        doc, doc_schema = to_visit.popleft()
+        for key, definition in doc_schema.items():
+            if definition.get('type') == 'dict' and definition.get('schema'):
+                # This is a subdocument with its own schema, visit it later.
+                subdoc = doc.get(key)
+                if not subdoc:
+                    continue
+                to_visit.append((subdoc, definition['schema']))
+                continue
+            if definition.get('coerce') != 'markdown':
+                continue
+
+            my_log.debug('I have to change %r of %s', key, doc)
+            old_value = doc.get(key)
+            if not old_value:
+                continue
+            new_value = callback(old_value)
+            doc[key] = new_value
+
+
+@manager_maintenance.option('-p', '--project', dest='proj_url', nargs='?',
+                            help='Project URL')
+@manager_maintenance.option('-a', '--all', dest='all_projects', action='store_true', default=False,
+                            help='Replace on all projects.')
+@manager_maintenance.option('-g', '--go', dest='go', action='store_true', default=False,
+                            help='Actually perform the changes (otherwise just show as dry-run).')
+def upgrade_attachment_usage(proj_url=None, all_projects=False, go=False):
+    """Replaces '@[slug]' with '{attachment slug}'.
+
+    Also moves links from the attachment dict to the attachment shortcode.
+    """
+    if bool(proj_url) == all_projects:
+        log.error('Use either --project or --all.')
+        return 1
+
+    import html
+    from pillar.api.node_types import ATTACHMENT_SLUG_REGEX
+    from pillar.api.projects.utils import node_type_dict
+    from pillar.api.utils import remove_private_keys
+    from pillar.api.utils.authentication import force_cli_user
+
+    force_cli_user()
+
+    nodes_coll = current_app.db('nodes')
+    total_nodes = 0
+    old_slug_re = re.compile(r'@\[(%s)\]' % ATTACHMENT_SLUG_REGEX)
+    for proj in _db_projects(proj_url, all_projects, go=go):
+        proj_id = proj['_id']
+        proj_url = proj.get('url', '-no-url-')
+        nodes = nodes_coll.find({
+            '_deleted': {'$ne': True},
+            'project': proj_id,
+            'properties.attachments': {'$exists': True},
+        })
+        node_count = nodes.count()
+        if node_count == 0:
+            log.debug('Skipping project %s (%s)', proj_url, proj_id)
+            continue
+        total_nodes += node_count
+
+        proj_node_types = node_type_dict(proj)
+
+        for node in nodes:
+            attachments = node['properties']['attachments']
+
+            # Inner functions because of access to the node's attachments.
+            def replace(match):
+                slug = match.group(1)
+                log.debug('    - OLD STYLE attachment slug %r', slug)
+                try:
+                    att = attachments[slug]
+                except KeyError:
+                    log.info("Attachment %r not found for node %s", slug, node['_id'])
+                    link = ''
+                else:
+                    link = att.get('link', '')
+                    if link == 'self':
+                        link = " link='self'"
+                    elif link == 'custom':
+                        url = att.get('link_custom')
+                        if url:
+                            link = " link='%s'" % html.escape(url)
+                return '{attachment %r%s}' % (slug, link)
+
+            def update_markdown(value: str) -> str:
+                return old_slug_re.sub(replace, value)
+
+            iter_markdown(proj_node_types, node, update_markdown)
+
+            # Remove no longer used properties from attachments
+            for attachment in attachments.values():
+                attachment.pop('link', None)
+                attachment.pop('link_custom', None)
+
+            if go:
+                # Use Eve to PUT, so we have schema checking.
+                db_node = remove_private_keys(node)
+                r, _, _, status = current_app.put_internal('nodes', db_node, _id=node['_id'])
+                if status != 200:
+                    log.error('Error %i storing altered node %s %s', status, node['_id'], r)
+                    raise SystemExit('Error storing node; see log.')
+
+        log.info('Project %s (%s) has %d nodes with attachments',
+                 proj_url, proj_id, node_count)
+    if not go:
+        log.info('Would update %d nodes', total_nodes)
+
+
+def _db_projects(proj_url: str, all_projects: bool, project_id='', *, go: bool) \
+        -> typing.Iterable[dict]:
+    """Yields a subset of the projects in the database.
+
+    :param all_projects: when True, yields all projects.
+    :param proj_url: when all_projects is False, this denotes the project
+        to yield.
+
+    Handles soft-deleted projects as non-existing. This ensures that
+    the receiver can actually modify and save the project without any
+    issues.
+
+    Also shows duration and a note about dry-running when go=False.
+    """
+    if sum([bool(proj_url), all_projects, bool(project_id)]) != 1:
+        log.error('Only use one way to specify a project / all projects')
+        raise SystemExit(1)
+
+    projects_coll = current_app.db('projects')
+    start = datetime.datetime.now()
     if all_projects:
-        for proj in projects_coll.find():
+        yield from projects_coll.find({'_deleted': {'$ne': True}})
-            handle_project(proj)
+    else:
-        return
+        if proj_url:
-
+            q = {'url': proj_url}
-    proj = projects_coll.find_one({'url': proj_url})
+        else:
+            q = {'_id': bson.ObjectId(project_id)}
+        proj = projects_coll.find_one({**q, '_deleted': {'$ne': True}})
         if not proj:
-        log.error('Project url=%s not found', proj_url)
+            log.error('Project %s not found', q)
-        return 3
+            raise SystemExit(3)
+        yield proj
 
-    handle_project(proj)
+    if not go:
+        log.info('Dry run, use --go to perform the change.')
+    duration = datetime.datetime.now() - start
+    log.info('Command took %s', duration)
 
 
 def _find_orphan_files() -> typing.Set[bson.ObjectId]:

pillar/shortcodes.py

@@ -21,6 +21,7 @@ import typing
 import urllib.parse
 
 import shortcodes
+import pillarsdk
 
 _parser: shortcodes.Parser = None
 _commented_parser: shortcodes.Parser = None
@@ -156,6 +157,105 @@ def iframe(context: typing.Any,
     return html
 
 
+@shortcode('attachment')
+class Attachment:
+    class NoSuchSlug(ValueError):
+        """Raised when there is no attachment with the given slug."""
+
+    class NoSuchFile(ValueError):
+        """Raised when the file pointed to by the attachment doesn't exist."""
+
+    class NotSupported(ValueError):
+        """Raised when an attachment is not pointing to a file."""
+
+    def __call__(self,
+                 context: typing.Any,
+                 content: str,
+                 pargs: typing.List[str],
+                 kwargs: typing.Dict[str, str]) -> str:
+        if isinstance(context, pillarsdk.Resource):
+            context = context.to_dict()
+        if not isinstance(context, dict):
+            return '{attachment context not a dictionary}'
+
+        try:
+            slug = pargs[0]
+        except KeyError:
+            return '{attachment No slug given}'
+
+        try:
+            file_doc = self.sdk_file(slug, context)
+        except self.NoSuchSlug:
+            return html_module.escape('{attachment %r does not exist}' % slug)
+        except self.NoSuchFile:
+            return html_module.escape('{attachment file for %r does not exist}' % slug)
+        except self.NotSupported as ex:
+            return html_module.escape('{attachment %s}' % ex)
+
+        return self.render(file_doc, kwargs)
+
+    def sdk_file(self, slug: str, node_properties: dict) -> pillarsdk.File:
+        """Return the file document for the attachment with this slug."""
+
+        from pillar.web import system_util
+
+        attachments = node_properties.get('attachments', {})
+        attachment = attachments.get(slug)
+        if not attachment:
+            raise self.NoSuchSlug(slug)
+
+        object_id = attachment.get('oid')
+        if not object_id:
+            raise self.NoSuchFile(object_id)
+
+        # In theory attachments can also point to other collections.
+        # There is no support for that yet, though.
+        collection = attachment.get('collection', 'files')
+        if collection != 'files':
+            log.warning('Attachment %r points to ObjectID %s in unsupported collection %r',
+                        slug, object_id, collection)
+            raise self.NotSupported(f'unsupported collection {collection!r}')
+
+        api = system_util.pillar_api()
+        sdk_file = pillarsdk.File.find(object_id, api=api)
+        return sdk_file
+
+    def render(self, sdk_file: pillarsdk.File, tag_args: dict) -> str:
+        file_renderers = {
+            'image': self.render_image,
+            'video': self.render_video,
+        }
+
+        mime_type_cat, _ = sdk_file.content_type.split('/', 1)
+        renderer = file_renderers.get(mime_type_cat, self.render_generic)
+        return renderer(sdk_file, tag_args)
+
+    def render_generic(self, sdk_file, tag_args):
+        import flask
+        return flask.render_template('nodes/attachments/file_generic.html',
+                                     file=sdk_file, tag_args=tag_args)
+
+    def render_image(self, sdk_file, tag_args):
+        """Renders an image file."""
+        import flask
+        variations = {var.size: var for var in sdk_file.variations}
+        return flask.render_template('nodes/attachments/file_image.html',
+                                     file=sdk_file, vars=variations, tag_args=tag_args)
+
+    def render_video(self, sdk_file, tag_args):
+        """Renders a video file."""
+        import flask
+        try:
+            # The very first variation is an mp4 file with max width of 1920px
+            default_variation = sdk_file.variations[0]
+        except IndexError:
+            log.error('Could not find variations for file %s' % sdk_file._id)
+            return flask.render_template('nodes/attachments/file_generic.html', file=sdk_file)
+
+        return flask.render_template('nodes/attachments/file_video.html',
+                                     file=sdk_file, var=default_variation, tag_args=tag_args)
+
+
 def _get_parser() -> typing.Tuple[shortcodes.Parser, shortcodes.Parser]:
     """Return the shortcodes parser, create it if necessary."""
     global _parser, _commented_parser
@@ -180,6 +280,7 @@ def render_commented(text: str, context: typing.Any = None) -> str:
     except shortcodes.InvalidTagError as ex:
         return html_module.escape('{%s}' % ex)
     except shortcodes.RenderingError as ex:
+        log.info('Error rendering tag', exc_info=True)
         return html_module.escape('{unable to render tag: %s}' % str(ex.__cause__ or ex))
 
 

pillar/web/nodes/attachments.py

@@ -1,109 +1,14 @@
-import logging
+"""Attachment form handling."""
-import re
+
+import logging
 
-from bson import ObjectId
-import flask
-import pillarsdk
 import wtforms
 
-from pillar.api.node_types import ATTACHMENT_SLUG_REGEX
-from pillar.web.utils import system_util
 from pillar.web.utils.forms import build_file_select_form, CustomFormField
 
-shortcode_re = re.compile(r'@\[(%s)\]' % ATTACHMENT_SLUG_REGEX)
 log = logging.getLogger(__name__)
 
 
-def render_attachments(node, field_value):
-    """Renders attachments referenced in the field value.
-
-    Returns the rendered field.
-    """
-
-    # TODO: cache this based on the node's etag and attachment links expiry.
-
-    node_attachments = node.properties.attachments or {}
-    if isinstance(node_attachments, list):
-        log.warning('Old-style attachments property found on node %s. Ignoring them, '
-                    'will result in attachments not being found.', node['_id'])
-        return field_value
-
-    if not node_attachments:
-        return field_value
-
-    def replace(match):
-        slug = match.group(1)
-
-        try:
-            att = node_attachments[slug]
-        except KeyError:
-            return '[attachment "%s" not found]' % slug
-        return render_attachment(att)
-
-    return shortcode_re.sub(replace, field_value)
-
-
-def render_attachment(attachment):
-    """Renders an attachment as HTML"""
-
-    oid = ObjectId(attachment['oid'])
-    collection = attachment.collection or 'files'
-
-    renderers = {
-        'files': render_attachment_file
-    }
-
-    try:
-        renderer = renderers[collection]
-    except KeyError:
-        log.error('Unable to render attachment from collection %s', collection)
-        return 'Unable to render attachment'
-
-    return renderer(attachment)
-
-
-def render_attachment_file(attachment):
-    """Renders a file attachment."""
-
-    api = system_util.pillar_api()
-    sdk_file = pillarsdk.File.find(attachment['oid'], api=api)
-
-    file_renderers = {
-        'image': render_attachment_file_image,
-        'video': render_attachment_file_video,
-    }
-
-    mime_type_cat, _ = sdk_file.content_type.split('/', 1)
-    try:
-        renderer = file_renderers[mime_type_cat]
-    except KeyError:
-        return flask.render_template('nodes/attachments/file_generic.html', file=sdk_file)
-
-    return renderer(sdk_file, attachment)
-
-
-def render_attachment_file_image(sdk_file, attachment):
-    """Renders an image file."""
-
-    variations = {var.size: var for var in sdk_file.variations}
-    return flask.render_template('nodes/attachments/file_image.html',
-                                 file=sdk_file, vars=variations, attachment=attachment)
-
-
-def render_attachment_file_video(sdk_file, attachment):
-    """Renders a video file."""
-
-    try:
-        # The very first variation is an mp4 file with max width of 1920px
-        default_variation = sdk_file.variations[0]
-    except IndexError:
-        log.error('Could not find variations for file %s' % sdk_file._id)
-        return flask.render_template('nodes/attachments/file_generic.html', file=sdk_file)
-
-    return flask.render_template('nodes/attachments/file_video.html',
-                                 file=sdk_file, var=default_variation, attachment=attachment)
-
-
 def attachment_form_group_create(schema_prop):
     """Creates a wtforms.FieldList for attachments."""
 
@@ -118,8 +23,6 @@ def _attachment_build_single_field(schema_prop):
     fake_schema = {
         'slug': schema_prop['propertyschema'],
         'oid': schema_prop['valueschema']['schema']['oid'],
-        'link': schema_prop['valueschema']['schema']['link'],
-        'link_custom': schema_prop['valueschema']['schema']['link_custom'],
     }
     file_select_form_group = build_file_select_form(fake_schema)
     return file_select_form_group
@@ -141,16 +44,10 @@ def attachment_form_group_set_data(db_prop_value, schema_prop, field_list):
         # Even uglier hard-coded
         subform.slug = slug
         subform.oid = att_data['oid']
-        subform.link = 'self'
-        subform.link_custom = None
-        if 'link' in att_data:
-            subform.link = att_data['link']
-        if 'link_custom' in att_data:
-            subform.link_custom = att_data['link_custom']
         field_list.append_entry(subform)
 
 
-def attachment_form_parse_post_data(data):
+def attachment_form_parse_post_data(data) -> dict:
     """Returns a dict that can be stored in the node.properties.attachments."""
 
     attachments = {}
@@ -159,18 +56,12 @@ def attachment_form_parse_post_data(data):
     for allprops in data:
         oid = allprops['oid']
         slug = allprops['slug']
-        link = allprops['link']
-        link_custom = allprops['link_custom']
 
-        if not allprops['slug'] and not oid:
+        if not allprops['slug'] or not oid:
             continue
 
         if slug in attachments:
             raise ValueError('Slug "%s" is used more than once' % slug)
         attachments[slug] = {'oid': oid}
-        attachments[slug]['link'] = link
-
-        if link == 'custom':
-            attachments[slug]['link_custom'] = link_custom
 
     return attachments

pillar/web/nodes/custom/posts.py

@@ -86,19 +86,11 @@ def posts_view(project_id=None, project_url=None, url=None, *, archive=False, pa
 
     if post is not None:
         # If post is not published, check that the user is also the author of
-        # the post. If not, return 404.
+        # the post. If not, return an error.
         if post.properties.status != "published":
             if not (current_user.is_authenticated and post.has_method('PUT')):
                 abort(403)
 
-        try:
-            post_contents = post['properties']['content']
-        except KeyError:
-            log.warning('Blog post %s has no content', post._id)
-        else:
-            post['properties']['content'] = pillar.web.nodes.attachments.render_attachments(
-                post, post_contents)
-
     can_create_blog_posts = project.node_type_has_method('post', 'POST', api=api)
 
     # Use functools.partial so we can later pass page=X.

pillar/web/nodes/forms.py

@@ -169,6 +169,8 @@ def process_node_form(form, node_id=None, node_type=None, user=None):
             data = form[prop_name].data
             if schema_prop['type'] == 'dict':
                 data = attachments.attachment_form_parse_post_data(data)
+                if not data:
+                    data = None
             elif schema_prop['type'] == 'integer':
                 if not data:
                     data = None

pillar/web/nodes/routes.py

@@ -177,9 +177,6 @@ def view(node_id, extra_template_args: dict=None):
     for child in children:
         child.picture = get_file(child.picture, api=api)
 
-    if 'description' in node:
-        node['description'] = attachments.render_attachments(node, node['description'])
-
     # Overwrite the file length by the biggest variation, if any.
     if node.file and node.file_variations:
         node.file.length = max(var.length for var in node.file_variations)

src/templates/nodes/attachments/file_image.pug

@@ -1,9 +1,9 @@
-| {% if 'link' in attachment and attachment['link'] != 'none' %}
+| {% if 'link' in tag_args and tag_args['link'] != 'none' %}
-| {% if attachment['link'] == 'self' %}
+|     {% if tag_args['link'] == 'self' %}
 a(href="{{ vars['l'].link }}")
 	img(src="{{ vars['l'].link }}", alt="{{ file.filename }}")
-| {% elif attachment['link'] == 'custom' %}
+|     {% else %}
-a(href="{{ attachment['link_custom'] }}")
+a(href="{{ tag_args['link'] }}", target="_blank")
 	img(src="{{ vars['l'].link }}", alt="{{ file.filename }}")
 |     {% endif %}
 | {% else %}

tests/test_api/test_cli.py

@@ -329,7 +329,7 @@ class UpgradeAttachmentSchemaTest(AbstractNodeReplacementTest):
         group_perms = self.add_group_permission_to_asset_node_type()
 
         with self.app.test_request_context():
-            upgrade_attachment_schema(self.proj['url'])
+            upgrade_attachment_schema(self.proj['url'], go=True)
 
         dbproj = self.fetch_project_from_db()
 

tests/test_cli/test_maintenance.py

@@ -1,6 +1,7 @@
 from bson import ObjectId
 
 from pillar.tests import AbstractPillarTest
+from pillar.tests import common_test_data as ctd
 
 
 class PurgeHomeProjectsTest(AbstractPillarTest):
@@ -33,3 +34,144 @@ class PurgeHomeProjectsTest(AbstractPillarTest):
             proj_coll = self.app.db('projects')
             self.assertEqual(True, proj_coll.find_one({'_id': ObjectId(home_a['_id'])})['_deleted'])
             self.assertEqual(True, proj_coll.find_one({'_id': ObjectId(home_b['_id'])})['_deleted'])
+
+
+class UpgradeAttachmentUsageTest(AbstractPillarTest):
+    def setUp(self, **kwargs):
+        super().setUp(**kwargs)
+        self.pid, self.uid = self.create_project_with_admin(user_id=24 * 'a')
+
+        with self.app.app_context():
+            files_coll = self.app.db('files')
+
+            res = files_coll.insert_one({
+                **ctd.EXAMPLE_FILE,
+                'project': self.pid,
+                'user': self.uid,
+            })
+            self.fid = res.inserted_id
+
+    def test_image_link(self):
+        with self.app.app_context():
+            nodes_coll = self.app.db('nodes')
+            res = nodes_coll.insert_one({
+                **ctd.EXAMPLE_NODE,
+                'picture': self.fid,
+                'project': self.pid,
+                'user': self.uid,
+                'description': "# Title\n\n@[slug0]\n@[slug1]\n@[slug2]\nEitje van Fabergé.",
+                'properties': {
+                    'status': 'published',
+                    'content_type': 'image',
+                    'file': self.fid,
+                    'attachments': {
+                        'slug0': {
+                            'oid': self.fid,
+                            'link': 'self',
+                        },
+                        'slug1': {
+                            'oid': self.fid,
+                            'link': 'custom',
+                            'link_custom': 'https://cloud.blender.org/',
+                        },
+                        'slug2': {
+                            'oid': self.fid,
+                        },
+                    }
+                }
+            })
+            nid = res.inserted_id
+
+        from pillar.cli.maintenance import upgrade_attachment_usage
+
+        with self.app.app_context():
+            upgrade_attachment_usage(proj_url=ctd.EXAMPLE_PROJECT['url'], go=True)
+            node = nodes_coll.find_one({'_id': nid})
+
+            self.assertEqual(
+                "# Title\n\n"
+                "{attachment 'slug0' link='self'}\n"
+                "{attachment 'slug1' link='https://cloud.blender.org/'}\n"
+                "{attachment 'slug2'}\n"
+                "Eitje van Fabergé.",
+                node['description'],
+                'The description should be updated')
+            self.assertEqual(
+                "<h1>Title</h1>\n"
+                "<!-- {attachment 'slug0' link='self'} -->\n"
+                "<!-- {attachment 'slug1' link='https://cloud.blender.org/'} -->\n"
+                "<!-- {attachment 'slug2'} -->\n"
+                "<p>Eitje van Fabergé.</p>\n",
+                node['_description_html'],
+                'The _description_html should be updated')
+
+            self.assertEqual(
+                {'slug0': {'oid': self.fid},
+                 'slug1': {'oid': self.fid},
+                 'slug2': {'oid': self.fid},
+                 },
+                node['properties']['attachments'],
+                'The link should have been removed from the attachment')
+
+    def test_post(self):
+        """This requires checking the dynamic schema of the node."""
+        with self.app.app_context():
+            nodes_coll = self.app.db('nodes')
+            res = nodes_coll.insert_one({
+                **ctd.EXAMPLE_NODE,
+                'node_type': 'post',
+                'project': self.pid,
+                'user': self.uid,
+                'picture': self.fid,
+                'description': "meh",
+                'properties': {
+                    'status': 'published',
+                    'content': "# Title\n\n@[slug0]\n@[slug1]\n@[slug2]\nEitje van Fabergé.",
+                    'attachments': {
+                        'slug0': {
+                            'oid': self.fid,
+                            'link': 'self',
+                        },
+                        'slug1': {
+                            'oid': self.fid,
+                            'link': 'custom',
+                            'link_custom': 'https://cloud.blender.org/',
+                        },
+                        'slug2': {
+                            'oid': self.fid,
+                        },
+                    }
+                }
+            })
+            nid = res.inserted_id
+
+        from pillar.cli.maintenance import upgrade_attachment_usage
+
+        with self.app.app_context():
+            upgrade_attachment_usage(proj_url=ctd.EXAMPLE_PROJECT['url'], go=True)
+            node = nodes_coll.find_one({'_id': nid})
+
+            self.assertEqual(
+                "# Title\n\n"
+                "{attachment 'slug0' link='self'}\n"
+                "{attachment 'slug1' link='https://cloud.blender.org/'}\n"
+                "{attachment 'slug2'}\n"
+                "Eitje van Fabergé.",
+                node['properties']['content'],
+                'The content should be updated')
+            self.assertEqual(
+                "<h1>Title</h1>\n"
+                "<!-- {attachment 'slug0' link='self'} -->\n"
+                "<!-- {attachment 'slug1' link='https://cloud.blender.org/'} -->\n"
+                "<!-- {attachment 'slug2'} -->\n"
+                "<p>Eitje van Fabergé.</p>\n",
+                node['properties']['_content_html'],
+                'The _content_html should be updated')
+
+            self.assertEqual(
+                {'slug0': {'oid': self.fid},
+                 'slug1': {'oid': self.fid},
+                 'slug2': {'oid': self.fid},
+                 },
+                node['properties']['attachments'],
+                'The link should have been removed from the attachment')