Merge pull request #239 from taigaio/bug/2083/user-story-history-repeats-files-and-has-no-order

bameda · bameda · commit 2d69b244b6e7 · 2015-02-11T11:33:38.000+01:00
Locking the element on take_snapshot to avoid racing condition issues
diff --git a/requirements.txt b/requirements.txt
@@ -30,6 +30,7 @@ django-ipware==0.1.0
 premailer==2.8.1
 django-transactional-cleanup==0.1.13
 lxml==3.4.1
+git+https://github.com/Xof/django-pglocks.git@dbb8d7375066859f897604132bd437832d2014ea
 
 # Comment it if you are using python >= 3.4
 enum34==1.0
diff --git a/taiga/projects/history/services.py b/taiga/projects/history/services.py
@@ -37,6 +37,7 @@ def create(request):
 from django.core.paginator import Paginator, InvalidPage
 from django.apps import apps
 from django.db import transaction as tx
+from django_pglocks import advisory_lock
 
 from taiga.mdrender.service import render as mdrender
 from taiga.base.utils.db import get_typename_for_model_class
@@ -269,6 +270,7 @@ def get_modified_fields(obj:object, last_modifications):
 
     return modified_fields
 
+
 @tx.atomic
 def take_snapshot(obj:object, *, comment:str="", user=None, delete:bool=False):
     """
@@ -280,56 +282,57 @@ def take_snapshot(obj:object, *, comment:str="", user=None, delete:bool=False):
     """
 
     key = make_key_from_model_object(obj)
-    typename = get_typename_for_model_class(obj.__class__)
-
-    new_fobj = freeze_model_instance(obj)
-    old_fobj, need_real_snapshot = get_last_snapshot_for_key(key)
-
-    entry_model = apps.get_model("history", "HistoryEntry")
-    user_id = None if user is None else user.id
-    user_name = "" if user is None else user.get_full_name()
-
-    # Determine history type
-    if delete:
-        entry_type = HistoryType.delete
-    elif new_fobj and not old_fobj:
-        entry_type = HistoryType.create
-    elif new_fobj and old_fobj:
-        entry_type = HistoryType.change
-    else:
-        raise RuntimeError("Unexpected condition")
-
-    fdiff = make_diff(old_fobj, new_fobj)
-
-    # If diff and comment are empty, do
-    # not create empty history entry
-    if (not fdiff.diff and not comment
-        and old_fobj is not None
-        and entry_type != HistoryType.delete):
-
-        return None
-
-    fvals = make_diff_values(typename, fdiff)
-
-    if len(comment) > 0:
-        is_hidden = False
-    else:
-        is_hidden = is_hidden_snapshot(fdiff)
-
-    kwargs = {
-        "user": {"pk": user_id, "name": user_name},
-        "key": key,
-        "type": entry_type,
-        "snapshot": fdiff.snapshot if need_real_snapshot else None,
-        "diff": fdiff.diff,
-        "values": fvals,
-        "comment": comment,
-        "comment_html": mdrender(obj.project, comment),
-        "is_hidden": is_hidden,
-        "is_snapshot": need_real_snapshot,
-    }
-
-    return entry_model.objects.create(**kwargs)
+    with advisory_lock(key) as acquired_key_lock:
+        typename = get_typename_for_model_class(obj.__class__)
+
+        new_fobj = freeze_model_instance(obj)
+        old_fobj, need_real_snapshot = get_last_snapshot_for_key(key)
+
+        entry_model = apps.get_model("history", "HistoryEntry")
+        user_id = None if user is None else user.id
+        user_name = "" if user is None else user.get_full_name()
+
+        # Determine history type
+        if delete:
+            entry_type = HistoryType.delete
+        elif new_fobj and not old_fobj:
+            entry_type = HistoryType.create
+        elif new_fobj and old_fobj:
+            entry_type = HistoryType.change
+        else:
+            raise RuntimeError("Unexpected condition")
+
+        fdiff = make_diff(old_fobj, new_fobj)
+
+        # If diff and comment are empty, do
+        # not create empty history entry
+        if (not fdiff.diff and not comment
+            and old_fobj is not None
+            and entry_type != HistoryType.delete):
+
+            return None
+
+        fvals = make_diff_values(typename, fdiff)
+
+        if len(comment) > 0:
+            is_hidden = False
+        else:
+            is_hidden = is_hidden_snapshot(fdiff)
+
+        kwargs = {
+            "user": {"pk": user_id, "name": user_name},
+            "key": key,
+            "type": entry_type,
+            "snapshot": fdiff.snapshot if need_real_snapshot else None,
+            "diff": fdiff.diff,
+            "values": fvals,
+            "comment": comment,
+            "comment_html": mdrender(obj.project, comment),
+            "is_hidden": is_hidden,
+            "is_snapshot": need_real_snapshot,
+        }
+
+        return entry_model.objects.create(**kwargs)
 
 
 # High level query api
