Update docs

Co-authored-by: Richard Schreiber <schreiber@rami.io>

doc/development/api/plugins.rst

@@ -121,7 +121,7 @@ This will automatically make pretix discover this plugin as soon as it is instal
 through ``pip``. During development, you can just run ``python setup.py develop`` inside
 your plugin source directory to make it discoverable.
 
-.. _`Signals`:
+.. _`signals`:
 Signals
 -------
 
@@ -154,7 +154,7 @@ in the ``installed`` method:
 Note that ``installed`` will *not* be called if the plugin is indirectly activated for an event
 because the event is created with settings copied from another event.
 
-.. _`Registries`:
+.. _`registries`:
 Registries
 ----------
 
@@ -163,18 +163,15 @@ ticket renderer.
 
 However, for some of them (types of :ref:`Log Entries <logging>`) we use a different method to keep track of them:
 In a ``Registry``, classes are collected at application startup, along with a unique key (in case
-of LogEntryType, the action_type) as well as which plugin registered them.
+of LogEntryType, the ``action_type``) as well as which plugin registered them.
 
-To register a class, you can use one of several decorator provided by the Registry object:
+To register a class, you can use one of several decorators provided by the Registry object:
 
-.. code-block:: python
-
-   @log_entry_types.new('my_pretix_plugin.some.action', _('Some action in My Pretix Plugin occured.'))
-   class MyPretixPluginLogEntryType(EventLogEntryType):
-       pass
+.. autoclass:: pretix.base.logentrytypes.LogEntryTypeRegistry
+   :members: register, new, new_from_dict
 
 All files in which classes are registered need to be imported in the ``AppConfig.ready`` as explained
-in `Signals`_ above.
+in `Signals <signals>`_ above.
 
 Views
 -----

doc/development/implementation/logging.rst

@@ -73,7 +73,7 @@ following ready-to-include template::
    {% include "pretixcontrol/includes/logs.html" with obj=order %}
 
 We now need a way to translate the action codes like ``pretix.event.changed`` into human-readable
-strings. The :py:attr:`pretix.base.logentrytypes.log_entry_types` :ref:`Registry <Registries>` allows you to do so. A simple
+strings. The :py:attr:`pretix.base.logentrytypes.log_entry_types` :ref:`registry <registries>` allows you to do so. A simple
 implementation could look like:
 
 .. code-block:: python
@@ -90,43 +90,58 @@ implementation could look like:
     class CoreOrderLogEntryType(OrderLogEntryType):
         pass
 
-Please note that you always need to define your own inherited LogEntryType class in your plugin. If you would just
-register an instance of a LogEntryType class defined in pretix core, it is not correctly registered as belonging to
-your plugin, leading to confusing user interface situations.
+Please note that you always need to define your own inherited ``LogEntryType`` class in your plugin. If you would just
+register an instance of a ``LogEntryType`` class defined in pretix core, it cannot be automatically detected as belonging
+to your plugin, leading to confusing user interface situations.
 
 
 Customizing log entry display
-""""""""""""""""""""""""""""""""""
+"""""""""""""""""""""""""""""
 
-The base LogEntryType classes allows for varying degree of customization in their descendants.
+The base ``LogEntryType`` classes allow for varying degree of customization in their descendants.
 
-If you want to add another log message for an existing core object (e.g. an Order, Item or Voucher), you can inherit
-from its predefined LogEntryType, e.g. `OrderLogEntryType` and just specify a new plaintext string. You can use format
-strings to insert information from the LogEntry's `data` object as shown in the section above.
+If you want to add another log message for an existing core object (e.g. an :class:`Order <pretix.base.models.Order>`,
+:class:`Item <pretix.base.models.Item>`, or :class:`Voucher <pretix.base.models.Voucher>`), you can inherit
+from its predefined :class:`LogEntryType <pretix.base.logentrytypes.LogEntryType>`, e.g.
+:class:`OrderLogEntryType <pretix.base.logentrytypes.OrderLogEntryType>`, and just specify a new plaintext string.
+You can use format strings to insert information from the LogEntry's `data` object as shown in the section above.
+
+If you define a new model object in your plugin, you should make sure proper object links in the user interface are
+displayed for it. If your model object belongs logically to a pretix :class:`Event <pretix.base.models.Event>`, you can inherit from :class:`EventLogEntryType <pretix.base.logentrytypes.EventLogEntryType>`,
+and set the ``object_link_*`` fields accordingly. ``object_link_viewname`` refers to a django url name, which needs to
+accept the arguments `organizer` and `event`, containing the respective slugs, and additional arguments provided by
+``object_link_args``. The default implementation of ``object_link_args`` will return an argument named by
+````object_link_argname``, with a value of ``content_object.pk`` (the primary key of the model object).
+If you want to customize the name displayed for the object (instead of the result of calling ``str()`` on it),
+overwrite ``object_link_display_name``.
+
+
+.. code-block:: python
+
+    class ItemLogEntryType(EventLogEntryType):
+        object_link_wrapper = _('Product {val}')
+
+        # link will be generated as reverse('control:event.item', {'organizer': ..., 'event': ..., 'item': item.pk})
+        object_link_viewname = 'control:event.item'
+        object_link_argname = 'item'
 
-If you defined a new model object in your plugin, you should make sure proper object links in the user interface are
-displayed for it. If your model object belongs logically to a pretix `Event`, you can inherit from `EventLogEntryType`,
-and set the `object_link_*` fields accordingly. `object_link_viewname` refers to a django url name, which needs to
-accept the arguments `organizer` and `event`, containing the respective slugs, and an argument named by `object_link_argname`.
-The latter will contain the ID of the model object, if not customized by overriding `object_link_argvalue`.
-If you want to customize the name displayed for the object (instead of the result of calling `str()` on it),
-overwrite `object_link_display_name`.
 
 .. code-block:: python
 
     class OrderLogEntryType(EventLogEntryType):
         object_link_wrapper = _('Order {val}')
-        object_link_viewname = 'control:event.order'
-        object_link_argname = 'code'
 
-        def object_link_argvalue(self, order):
-            return order.code
+        # link will be generated as reverse('control:event.order', {'organizer': ..., 'event': ..., 'code': order.code})
+        object_link_viewname = 'control:event.order'
+
+        def object_link_args(self, order):
+            return {'code': order.code}
 
         def object_link_display_name(self, order):
             return order.code
 
-To show more sophisticated message strings, e.g. varying the message depending on information from the LogEntry's
-`data` object, overwrite the `display` method:
+To show more sophisticated message strings, e.g. varying the message depending on information from the :class:`LogEntry <pretix.base.models.log.LogEntry>`'s
+`data` object, override the `display` method:
 
 .. code-block:: python
 
@@ -145,15 +160,14 @@ To show more sophisticated message strings, e.g. varying the message depending o
 
 .. automethod:: pretix.base.logentrytypes.LogEntryType.display
 
-If your new model object does not belong to an `Event`, you need to implement
+If your new model object does not belong to an :class:`Event <pretix.base.models.Event>`, you need to inherit directly from ``LogEntryType`` instead
+of ``EventLogEntryType``, providing your own implementation of ``get_object_link_info`` if object links should be
+displayed.
 
-meow
+.. autoclass:: pretix.base.logentrytypes.LogEntryType
+   :members: get_object_link_info
 
-.. autoclass:: pretix.base.logentrytypes.Registry
-   :members: new
 
-.. autoclass:: pretix.base.logentrytypes.LogEntryTypeRegistry
-   :members: new, new_from_dict
 
 Sending notifications
 ---------------------

src/pretix/base/logentrytypes.py

@@ -53,9 +53,22 @@ def make_link(a_map, wrapper, is_active=True, event=None, plugin_name=None):
 
 
 class LogEntryTypeRegistry(EventPluginRegistry):
+    def __init__(self):
+        super().__init__({'action_type': lambda o: getattr(o, 'action_type')})
+
+    def register(self, *objs):
+        for obj in objs:
+            if not isinstance(obj, LogEntryType):
+                raise TypeError('Entries must be derived from LogEntryType')
+
+            if obj.__module__ == LogEntryType.__module__:
+                raise TypeError('Must not register base classes, only derived ones')
+
+        return super().register(*objs)
+
     def new_from_dict(self, data):
         """
-        Register multiple instance of a LogEntryType class with different action_type
+        Register multiple instance of a `LogEntryType` class with different `action_type`
         and plain text strings, as given by the items of the specified data dictionary.
 
         This method is designed to be used as a decorator as follows:
@@ -86,7 +99,7 @@ Registry for LogEntry types.
 Each entry in this registry should be an instance of a subclass of ``LogEntryType``.
 They are annotated with their ``action_type`` and the defining ``plugin``.
 """
-log_entry_types = LogEntryTypeRegistry({'action_type': lambda o: getattr(o, 'action_type')})
+log_entry_types = LogEntryTypeRegistry()
 
 
 class LogEntryType:
@@ -95,7 +108,6 @@ class LogEntryType:
     """
 
     def __init__(self, action_type=None, plain=None):
-        assert self.__module__ != LogEntryType.__module__  # must not instantiate base classes, only derived ones
         if action_type:
             self.action_type = action_type
         if plain:
@@ -117,12 +129,12 @@ class LogEntryType:
 
     def get_object_link_info(self, logentry) -> dict:
         """
-        Return information to generate a link to the content_object of a given logentry.
+        Return information to generate a link to the `content_object` of a given log entry.
 
         Not implemented in the base class, causing the object link to be omitted.
 
-        :return: `dict` with the keys `href` (containing a URL to view/edit the object) and `val` (containing the
-        escaped text for the anchor element)
+        :return: Dictionary with the keys ``href`` (containing a URL to view/edit the object) and ``val`` (containing the
+                 escaped text for the anchor element)
         """
         pass
 
@@ -142,23 +154,25 @@ class LogEntryType:
 
 class EventLogEntryType(LogEntryType):
     """
-    Base class for any LogEntry type whose content_object is either an `Event` itself or belongs to a specific `Event`.
+    Base class for any `LogEntry` type whose `content_object` is either an `Event` itself or belongs to a specific `Event`.
     """
 
     def get_object_link_info(self, logentry) -> dict:
-        if hasattr(self, 'object_link_viewname') and hasattr(self, 'object_link_argname') and logentry.content_object:
+        if hasattr(self, 'object_link_viewname') and logentry.content_object:
             return {
                 'href': reverse(self.object_link_viewname, kwargs={
                     'event': logentry.event.slug,
                     'organizer': logentry.event.organizer.slug,
-                    self.object_link_argname: self.object_link_argvalue(logentry.content_object),
+                    **self.object_link_args(logentry.content_object),
                 }),
                 'val': escape(self.object_link_display_name(logentry.content_object)),
             }
 
-    def object_link_argvalue(self, content_object):
-        """Return the identifier used in a link to content_object."""
-        return content_object.id
+    def object_link_args(self, content_object):
+        """Return the kwargs for the url used in a link to content_object."""
+        if hasattr(self, 'object_link_argname'):
+            return {self.object_link_argname: content_object.pk}
+        return {}
 
     def object_link_display_name(self, content_object):
         """Return the display name to refer to content_object in the user interface."""
@@ -168,10 +182,9 @@ class EventLogEntryType(LogEntryType):
 class OrderLogEntryType(EventLogEntryType):
     object_link_wrapper = _('Order {val}')
     object_link_viewname = 'control:event.order'
-    object_link_argname = 'code'
 
-    def object_link_argvalue(self, order):
-        return order.code
+    def object_link_args(self, order):
+        return {'code': order.code}
 
     def object_link_display_name(self, order):
         return order.code
@@ -183,7 +196,9 @@ class VoucherLogEntryType(EventLogEntryType):
     object_link_argname = 'voucher'
 
     def object_link_display_name(self, voucher):
-        return voucher.code[:6]
+        if len(voucher.code) > 6:
+            return voucher.code[:6] + "…"
+        return voucher.code
 
 
 class ItemLogEntryType(EventLogEntryType):

src/pretix/base/signals.py

@@ -254,11 +254,11 @@ class Registry:
 
     def __init__(self, keys):
         """
-        :param keys: dictionary {key: accessor_function}
+        :param keys: Dictionary with `{key: accessor_function}`
                      When a new entry is registered, all accessor functions are called with the new entry as parameter.
                      Their return value is stored as the metadata value for that key.
         """
-        self.registered_entries = list()
+        self.registered_entries = dict()
         self.keys = keys
         self.by_key = {key: {} for key in self.keys.keys()}
 
@@ -276,15 +276,21 @@ class Registry:
               # ...
         """
         for obj in objs:
+            if obj in self.registered_entries:
+                raise RuntimeError('Object already registered: {}'.format(obj))
+
             meta = {k: accessor(obj) for k, accessor in self.keys.items()}
             tup = (obj, meta)
             for key, value in meta.items():
                 self.by_key[key][value] = tup
-            self.registered_entries.append(tup)
+            self.registered_entries[obj] = meta
+
+        if len(objs) == 1:
+            return objs[0]
 
     def new(self, *args, **kwargs):
         """
-        Instantiate the decorated class with the given *args and **kwargs, and register the instance in this registry.
+        Instantiate the decorated class with the given `*args` and `**kwargs`, and register the instance in this registry.
         May be used multiple times.
 
         .. code-block:: python
@@ -306,10 +312,11 @@ class Registry:
         return self.by_key.get(key).get(value, (None, None))
 
     def filter(self, **kwargs):
-        return ((entry, meta)
-                for entry, meta in self.registered_entries
-                if all(value == meta[key] for key, value in kwargs.items())
-                )
+        return (
+            (entry, meta)
+            for entry, meta in self.registered_entries.items()
+            if all(value == meta[key] for key, value in kwargs.items())
+        )
 
 
 class EventPluginRegistry(Registry):
@@ -633,41 +640,16 @@ logentry_display = EventPluginSignal()
 """
 Arguments: ``logentry``
 
-To display an instance of the ``LogEntry`` model to a human user,
-``pretix.base.signals.logentry_display`` will be sent out with a ``logentry`` argument.
-
-The first received response that is not ``None`` will be used to display the log entry
-to the user. The receivers are expected to return plain text.
-
-As with all event-plugin signals, the ``sender`` keyword argument will contain the event.
+**DEPRECTATION:** Please do not use this signal for new LogEntry types. Use the log_entry_types
+registry instead, as described in https://docs.pretix.eu/en/latest/development/implementation/logging.html
 """
 
 logentry_object_link = EventPluginSignal()
 """
 Arguments: ``logentry``
 
-To display the relationship of an instance of the ``LogEntry`` model to another model
-to a human user, ``pretix.base.signals.logentry_object_link`` will be sent out with a
-``logentry`` argument.
-
-The first received response that is not ``None`` will be used to display the related object
-to the user. The receivers are expected to return a HTML link. The internal implementation
-builds the links like this::
-
-    a_text = _('Tax rule {val}')
-    a_map = {
-        'href': reverse('control:event.settings.tax.edit', kwargs={
-            'event': sender.slug,
-            'organizer': sender.organizer.slug,
-            'rule': logentry.content_object.id
-        }),
-        'val': escape(logentry.content_object.name),
-    }
-    a_map['val'] = '<a href="{href}">{val}</a>'.format_map(a_map)
-    return a_text.format_map(a_map)
-
-Make sure that any user content in the HTML code you return is properly escaped!
-As with all event-plugin signals, the ``sender`` keyword argument will contain the event.
+**DEPRECTATION:** Please do not use this signal for new LogEntry types. Use the log_entry_types
+registry instead, as described in https://docs.pretix.eu/en/latest/development/implementation/logging.html
 """
 
 requiredaction_display = EventPluginSignal()

src/tests/base/test_registry.py

@@ -0,0 +1,179 @@
+#
+# This file is part of pretix (Community Edition).
+#
+# Copyright (C) 2014-2020 Raphael Michel and contributors
+# Copyright (C) 2020-2021 rami.io GmbH and contributors
+#
+# This program is free software: you can redistribute it and/or modify it under the terms of the GNU Affero General
+# Public License as published by the Free Software Foundation in version 3 of the License.
+#
+# ADDITIONAL TERMS APPLY: Pursuant to Section 7 of the GNU Affero General Public License, additional terms are
+# applicable granting you additional permissions and placing additional restrictions on your usage of this software.
+# Please refer to the pretix LICENSE file to obtain the full terms applicable to this work. If you did not receive
+# this file, see <https://pretix.eu/about/en/license>.
+#
+# This program is distributed in the hope that it will be useful, but WITHOUT ANY WARRANTY; without even the implied
+# warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU Affero General Public License for more
+# details.
+#
+# You should have received a copy of the GNU Affero General Public License along with this program.  If not, see
+# <https://www.gnu.org/licenses/>.
+#
+from unittest import mock
+
+import pytest
+
+from pretix.base.logentrytypes import (
+    ItemLogEntryType, LogEntryType, LogEntryTypeRegistry,
+)
+from pretix.base.signals import Registry
+
+
+def test_registry_classes():
+    animal_type_registry = Registry({"type": lambda s: s.__name__, "classis": lambda s: s.classis})
+
+    @animal_type_registry.register
+    class Cat:
+        classis = 'mammalia'
+
+        def make_sound(self):
+            return "meow"
+
+    @animal_type_registry.register
+    class Dog:
+        classis = 'mammalia'
+
+        def make_sound(self):
+            return "woof"
+
+    @animal_type_registry.register
+    class Cricket:
+        classis = 'insecta'
+
+        def make_sound(self):
+            return "chirp"
+
+    # test retrieving and instantiating a class based on metadata value
+    clz, meta = animal_type_registry.get(type="Cat")
+    assert clz().make_sound() == "meow"
+    assert meta.get('type') == "Cat"
+
+    clz, meta = animal_type_registry.get(type="Dog")
+    assert clz().make_sound() == "woof"
+    assert meta.get('type') == "Dog"
+
+    # check that None is returned when no class exists with the specified metadata value
+    clz, meta = animal_type_registry.get(type="Unicorn")
+    assert clz is None
+    assert meta is None
+
+    # check that an error is raised when trying to retrieve by an undefined metadata key
+    with pytest.raises(Exception):
+        _, _ = animal_type_registry.get(whatever="hello")
+
+    # test finding all entries with a given metadata value
+    mammals = animal_type_registry.filter(classis='mammalia')
+    assert set(cls for cls, meta in mammals) == {Cat, Dog}
+    assert all(meta['classis'] == 'mammalia' for cls, meta in mammals)
+
+    insects = animal_type_registry.filter(classis='insecta')
+    assert set(cls for cls, meta in insects) == {Cricket}
+
+    fantasy = animal_type_registry.filter(classis='fantasia')
+    assert set(cls for cls, meta in fantasy) == set()
+
+    # check normal object instantiation still works with our decorator
+    assert Cat().make_sound() == "meow"
+
+
+def test_registry_instances():
+    animal_sound_registry = Registry({"animal": lambda s: s.animal})
+
+    @animal_sound_registry.new("dog", "woof")
+    @animal_sound_registry.new("cricket", "chirp")
+    class AnimalSound:
+        def __init__(self, animal, sound):
+            self.animal = animal
+            self.sound = sound
+
+        def make_sound(self):
+            return self.sound
+
+    @animal_sound_registry.new()
+    class CatSound(AnimalSound):
+        def __init__(self):
+            super().__init__(animal="cat", sound=["meow", "meww", "miaou"])
+            self.i = 0
+
+        def make_sound(self):
+            self.i += 1
+            return self.sound[self.i % len(self.sound)]
+
+    # test registry
+    assert animal_sound_registry.get(animal='dog')[0].make_sound() == "woof"
+    assert animal_sound_registry.get(animal='dog')[0].make_sound() == "woof"
+    assert animal_sound_registry.get(animal='cricket')[0].make_sound() == "chirp"
+    assert animal_sound_registry.get(animal='cat')[0].make_sound() == "meww"
+    assert animal_sound_registry.get(animal='cat')[0].make_sound() == "miaou"
+    assert animal_sound_registry.get(animal='cat')[0].make_sound() == "meow"
+
+    # check normal object instantiation still works with our decorator
+    assert AnimalSound("test", "test").make_sound() == "test"
+
+
+def test_registry_prevent_duplicates():
+    my_registry = Registry({"animal": lambda s: s.animal})
+
+    class AnimalSound:
+        def __init__(self, animal, sound):
+            self.animal = animal
+            self.sound = sound
+
+    cat = AnimalSound("cat", "meow")
+    my_registry.register(cat)
+
+    with pytest.raises(RuntimeError):
+        my_registry.register(cat)
+
+
+def test_logentrytype_registry():
+    reg = LogEntryTypeRegistry()
+
+    with mock.patch('pretix.base.signals.get_defining_app') as mock_get_defining_app:
+        mock_get_defining_app.return_value = 'my_plugin'
+
+        @reg.new("foo.mytype")
+        class MyType(LogEntryType):
+            pass
+
+    @reg.new("foo.myothertype")
+    class MyOtherType(LogEntryType):
+        pass
+
+    typ, meta = reg.get(action_type="foo.mytype")
+    assert isinstance(typ, MyType)
+    assert meta['action_type'] == "foo.mytype"
+    assert meta['plugin'] == 'my_plugin'
+
+    typ, meta = reg.get(action_type="foo.myothertype")
+    assert isinstance(typ, MyOtherType)
+    assert meta['action_type'] == "foo.myothertype"
+    assert meta['plugin'] is None
+
+    by_my_plugin = reg.filter(plugin='my_plugin')
+    assert set(type(typ) for typ, meta in by_my_plugin) == {MyType}
+
+
+def test_logentrytype_registry_validation():
+    reg = LogEntryTypeRegistry()
+
+    with pytest.raises(TypeError, match='Must not register base classes, only derived ones'):
+        reg.register(LogEntryType("foo.mytype"))
+
+    with pytest.raises(TypeError, match='Must not register base classes, only derived ones'):
+        reg.new_from_dict({"foo.mytype": "My Log Entry"})(ItemLogEntryType)
+
+    with pytest.raises(TypeError, match='Entries must be derived from LogEntryType'):
+        @reg.new()
+        class MyType:
+            pass