ic-labs / django-icekit

GLAMkit is a next-generation Python CMS by the Interaction Consortium, designed especially for the cultural sector.
http://glamkit.com
MIT License
47 stars 11 forks source link

Pages cannot be published if ATOMIC_REQUESTS is not True for the database. #132

Open Aramgutang opened 7 years ago

Aramgutang commented 7 years ago

Specifically, line 396 in icekit/publishing/admin.py will throw a TransactionManagementError stating "select_for_update cannot be used outside of a transaction":

def get_model_object(self, request, object_id):
    ...
    obj = self.model.objects.select_for_update().get(pk=object_id)

The view needs to be enclosed in a manual transaction to prevent such issues.

jmurty commented 7 years ago

I was wondering whether we should selective use select_for_update or not depending on the ATOMIC_REQUESTS project setting, but per the comment before the call this was added to avoid problems with multiple simultaneous publish/unpublish operations caused by users who double-click web form buttons so it would probably be best to keep it in all cases. Or find a more general solution to the problem of double-clickers:

        # Enforce DB-level locking of the object with `select_for_update` to
        # avoid data consistency issues caused by multiple simultaneous form
        # submission (e.g. by a user who double-clicks form buttons).
        obj = self.model.objects.select_for_update().get(pk=object_id)

Adding an atomic transaction decorator to the three views that use the method – revert_view, unpublish_view, and publish_view – should be enough to make it work in general.