Merge pull request #186 from taigaio/improving-occ-for-not-bothering-when-not-necesary

Improving OCC for detecting situations where the version is not valid bu...

taiga/projects/history/services.py

@@ -251,6 +251,24 @@ def get_last_snapshot_for_key(key:str) -> FrozenObj:
 
 # Public api
 
+def get_modified_fields(obj:object, last_modifications):
+    """
+    Get the modified fields for an object through his last modifications
+    """
+    key = make_key_from_model_object(obj)
+    entry_model = apps.get_model("history", "HistoryEntry")
+    history_entries = (entry_model.objects
+                                    .filter(key=key)
+                                    .order_by("-created_at")
+                                    .values_list("diff", flat=True)
+                                    [0:last_modifications])
+
+    modified_fields = []
+    for history_entry in history_entries:
+        modified_fields += history_entry.keys()
+
+    return modified_fields
+
 @tx.atomic
 def take_snapshot(obj:object, *, comment:str="", user=None, delete:bool=False):
     """

taiga/projects/occ/mixins.py

@@ -19,20 +19,55 @@ from django.utils.translation import ugettext_lazy as _
 
 from taiga.base import exceptions as exc
 from taiga.base.utils import db
-
+from taiga.projects.history.services import get_modified_fields
 
 class OCCResourceMixin(object):
     """
     Rest Framework resource mixin for resources that need to have concurrent
     accesses and editions controlled.
     """
+    def _extract_param_version(self):
+        param_version = self.request.DATA.get('version', None)
+        try:
+            param_version = param_version and int(param_version)
+        except (ValueError, TypeError):
+            raise exc.WrongArguments({"version": "The version must be an integer"})
+
+        return param_version
+
+    def _validate_param_version(self, param_version, current_version):
+        if param_version is not None:
+            if param_version < 0:
+                return False
+            if current_version is not None and param_version > current_version:
+                return False
+
+        return True
+
     def _validate_and_update_version(self, obj):
         current_version = None
         if obj.id:
             current_version = type(obj).objects.model.objects.get(id=obj.id).version
 
-        param_version = self.request.DATA.get('version', None)
+        # Extract param version
+        param_version = self._extract_param_version()
+        if not self._validate_param_version(param_version, current_version):
+            raise exc.WrongArguments({"version": "The version is not valid"})
+
         if current_version != param_version:
+            diff_versions = current_version - param_version
+
+            modifying_fields = set(self.request.DATA.keys())
+            if "version" in modifying_fields:
+                modifying_fields.remove("version")
+
+            modified_fields = set(get_modified_fields(obj, diff_versions))
+            if "version" in modifying_fields:
+                modified_fields.remove("version")
+
+            both_modified = modifying_fields & modified_fields
+
+            if both_modified:
                 raise exc.WrongArguments({"version": "The version doesn't match with the current one"})
 
         if obj.id:

tests/integration/test_occ.py

@@ -30,78 +30,6 @@ from .. import factories as f
 pytestmark = pytest.mark.django_db
 
 
-def test_invalid_concurrent_save_for_issue(client):
-    user = f.UserFactory.create()
-    project = f.ProjectFactory.create(owner=user)
-    membership = f.MembershipFactory.create(project=project, user=user, is_owner=True)
-    issue = f.IssueFactory.create(version=10, project=project)
-
-    client.login(user)
-
-    mock_path = "taiga.projects.issues.api.IssueViewSet.pre_conditions_on_save"
-    with patch(mock_path) as m:
-        url = reverse("issues-detail", args=(issue.id,))
-        data = {"version":9}
-        response = client.patch(url, json.dumps(data), content_type="application/json")
-        assert response.status_code == 400
-
-def test_valid_concurrent_save_for_issue(client):
-    user = f.UserFactory.create()
-    project = f.ProjectFactory.create(owner=user)
-    membership = f.MembershipFactory.create(project=project, user=user, is_owner=True)
-    issue = f.IssueFactory.create(version=10, project=project)
-
-    client.login(user)
-
-    mock_path = "taiga.projects.issues.api.IssueViewSet.pre_conditions_on_save"
-    with patch(mock_path) as m:
-        url = reverse("issues-detail", args=(issue.id,))
-        data = {"version": 10}
-        response = client.patch(url, json.dumps(data), content_type="application/json")
-        assert json.loads(response.content)['version'] == 11
-        assert response.status_code == 200
-        issue = Issue.objects.get(id=issue.id)
-        assert issue.version == 11
-
-def test_invalid_concurrent_save_for_wiki_page(client):
-    user = f.UserFactory.create()
-    project = f.ProjectFactory.create(owner=user)
-    membership = f.MembershipFactory.create(project=project, user=user, is_owner=True)
-    wiki_page = f.WikiPageFactory.create(version=10, project=project, owner=user)
-    client.login(user)
-
-    url = reverse("wiki-detail", args=(wiki_page.id,))
-    data = {"version":9}
-    response = client.patch(url, json.dumps(data), content_type="application/json")
-    assert response.status_code == 400
-
-def test_valid_concurrent_save_for_wiki_page(client):
-    user = f.UserFactory.create()
-    project = f.ProjectFactory.create(owner=user)
-    membership = f.MembershipFactory.create(project=project, user=user, is_owner=True)
-    wiki_page = f.WikiPageFactory.create(version=10, project=project, owner=user)
-    client.login(user)
-
-    url = reverse("wiki-detail", args=(wiki_page.id,))
-    data = {"version": 10}
-    response = client.patch(url, json.dumps(data), content_type="application/json")
-    assert json.loads(response.content)['version'] == 11
-    assert response.status_code == 200
-    wiki_page = WikiPage.objects.get(id=wiki_page.id)
-    assert wiki_page.version == 11
-
-def test_invalid_concurrent_save_for_us(client):
-    user = f.UserFactory.create()
-    project = f.ProjectFactory.create(owner=user)
-    membership = f.MembershipFactory.create(project=project, user=user, is_owner=True)
-    userstory = f.UserStoryFactory.create(version=10, project=project)
-    client.login(user)
-
-    url = reverse("userstories-detail", args=(userstory.id,))
-    data = {"version":9}
-    response = client.patch(url, json.dumps(data), content_type="application/json")
-    assert response.status_code == 400
-
 def test_valid_us_creation(client):
     user = f.UserFactory.create()
     project = f.ProjectFactory.create(owner=user)
@@ -118,48 +46,294 @@ def test_valid_us_creation(client):
     response = client.post(url, json.dumps(data), content_type="application/json")
     assert response.status_code == 201
 
-def test_valid_concurrent_save_for_us(client):
+
+def test_invalid_concurrent_save_for_issue(client):
+    user = f.UserFactory.create()
+    project = f.ProjectFactory.create(owner=user)
+    membership = f.MembershipFactory.create(project=project, user=user, is_owner=True)
+    client.login(user)
+
+    mock_path = "taiga.projects.issues.api.IssueViewSet.pre_conditions_on_save"
+    with patch(mock_path) as m:
+        url = reverse("issues-list")
+        data = {"subject": "test",
+                      "project": project.id,
+                      "status": f.IssueStatusFactory.create(project=project).id,
+                      "severity": f.SeverityFactory.create(project=project).id,
+                      "type": f.IssueTypeFactory.create(project=project).id,
+                      "priority": f.PriorityFactory.create(project=project).id}
+        response = client.json.post(url, json.dumps(data))
+        assert response.status_code == 201, response.content
+
+        issue_id = json.loads(response.content)["id"]
+        url = reverse("issues-detail", args=(issue_id,))
+        data = {"version":1, "subject": "test 1"}
+        response = client.patch(url, json.dumps(data), content_type="application/json")
+        assert response.status_code == 200
+
+        data = {"version":1, "subject": "test 2"}
+        response = client.patch(url, json.dumps(data), content_type="application/json")
+        assert response.status_code == 400
+
+
+def test_valid_concurrent_save_for_issue_different_versions(client):
+    user = f.UserFactory.create()
+    project = f.ProjectFactory.create(owner=user)
+    membership = f.MembershipFactory.create(project=project, user=user, is_owner=True)
+    client.login(user)
+
+    mock_path = "taiga.projects.issues.api.IssueViewSet.pre_conditions_on_save"
+    with patch(mock_path) as m:
+        url = reverse("issues-list")
+        data = {"subject": "test",
+                      "project": project.id,
+                      "status": f.IssueStatusFactory.create(project=project).id,
+                      "severity": f.SeverityFactory.create(project=project).id,
+                      "type": f.IssueTypeFactory.create(project=project).id,
+                      "priority": f.PriorityFactory.create(project=project).id}
+        response = client.json.post(url, json.dumps(data))
+        assert response.status_code == 201, response.content
+
+        issue_id = json.loads(response.content)["id"]
+        url = reverse("issues-detail", args=(issue_id,))
+        data = {"version":1, "subject": "test 1"}
+        response = client.patch(url, json.dumps(data), content_type="application/json")
+        assert response.status_code == 200
+
+        data = {"version":2, "subject": "test 2"}
+        response = client.patch(url, json.dumps(data), content_type="application/json")
+        assert response.status_code == 200
+
+
+def test_valid_concurrent_save_for_issue_different_fields(client):
+    user = f.UserFactory.create()
+    project = f.ProjectFactory.create(owner=user)
+    membership = f.MembershipFactory.create(project=project, user=user, is_owner=True)
+    client.login(user)
+
+    mock_path = "taiga.projects.issues.api.IssueViewSet.pre_conditions_on_save"
+    with patch(mock_path) as m:
+        url = reverse("issues-list")
+        data = {"subject": "test",
+                      "project": project.id,
+                      "status": f.IssueStatusFactory.create(project=project).id,
+                      "severity": f.SeverityFactory.create(project=project).id,
+                      "type": f.IssueTypeFactory.create(project=project).id,
+                      "priority": f.PriorityFactory.create(project=project).id}
+        response = client.json.post(url, json.dumps(data))
+        assert response.status_code == 201, response.content
+
+        issue_id = json.loads(response.content)["id"]
+        url = reverse("issues-detail", args=(issue_id,))
+        data = {"version":1, "subject": "test 1"}
+        response = client.patch(url, json.dumps(data), content_type="application/json")
+        assert response.status_code == 200
+
+        data = {"version":1, "description": "test 2"}
+        response = client.patch(url, json.dumps(data), content_type="application/json")
+        assert response.status_code == 200
+
+
+def test_invalid_concurrent_save_for_wiki_page(client):
+    user = f.UserFactory.create()
+    project = f.ProjectFactory.create(owner=user)
+    membership = f.MembershipFactory.create(project=project, user=user, is_owner=True)
+    client.login(user)
+
+    mock_path = "taiga.projects.wiki.api.WikiViewSet.pre_conditions_on_save"
+    with patch(mock_path) as m:
+        url = reverse("wiki-list")
+        data = {"project": project.id, "slug": "test"}
+        response = client.json.post(url, json.dumps(data))
+        assert response.status_code == 201, response.content
+
+        wiki_id = json.loads(response.content)["id"]
+        url = reverse("wiki-detail", args=(wiki_id,))
+        data = {"version":1, "content": "test 1"}
+        response = client.patch(url, json.dumps(data), content_type="application/json")
+        assert response.status_code == 200
+
+        data = {"version":1, "content": "test 2"}
+        response = client.patch(url, json.dumps(data), content_type="application/json")
+        assert response.status_code == 400
+
+
+def test_valid_concurrent_save_for_wiki_page_different_versions(client):
+    user = f.UserFactory.create()
+    project = f.ProjectFactory.create(owner=user)
+    membership = f.MembershipFactory.create(project=project, user=user, is_owner=True)
+    client.login(user)
+
+    mock_path = "taiga.projects.wiki.api.WikiViewSet.pre_conditions_on_save"
+    with patch(mock_path) as m:
+        url = reverse("wiki-list")
+        data = {"project": project.id, "slug": "test"}
+        response = client.json.post(url, json.dumps(data))
+        assert response.status_code == 201, response.content
+
+        wiki_id = json.loads(response.content)["id"]
+        url = reverse("wiki-detail", args=(wiki_id,))
+        data = {"version":1, "content": "test 1"}
+        response = client.patch(url, json.dumps(data), content_type="application/json")
+        assert response.status_code == 200
+
+        data = {"version":2, "content": "test 2"}
+        response = client.patch(url, json.dumps(data), content_type="application/json")
+        assert response.status_code == 200
+
+
+def test_invalid_concurrent_save_for_us(client):
     user = f.UserFactory.create()
     project = f.ProjectFactory.create(owner=user)
     membership = f.MembershipFactory.create(project=project, user=user, is_owner=True)
     userstory = f.UserStoryFactory.create(version=10, project=project)
     client.login(user)
 
-    url = reverse("userstories-detail", args=(userstory.id,))
+    mock_path = "taiga.projects.userstories.api.UserStoryViewSet.pre_conditions_on_save"
-    data = {"version": 10}
+    with patch(mock_path) as m:
+        url = reverse("userstories-list")
+        data = {"subject": "test",
+                      "project": project.id,
+                      "status": f.UserStoryStatusFactory.create(project=project).id}
+        response = client.json.post(url, json.dumps(data))
+        assert response.status_code == 201
+
+        userstory_id = json.loads(response.content)["id"]
+        url = reverse("userstories-detail", args=(userstory_id,))
+        data = {"version":1, "subject": "test 1"}
         response = client.patch(url, json.dumps(data), content_type="application/json")
-    assert json.loads(response.content)['version'] == 11
         assert response.status_code == 200
-    userstory = UserStory.objects.get(id=userstory.id)
+
-    assert userstory.version == 11
+        data = {"version":1, "subject": "test 2"}
+        response = client.patch(url, json.dumps(data), content_type="application/json")
+        assert response.status_code == 400
+
+
+def test_valid_concurrent_save_for_us_different_versions(client):
+    user = f.UserFactory.create()
+    project = f.ProjectFactory.create(owner=user)
+    membership = f.MembershipFactory.create(project=project, user=user, is_owner=True)
+    client.login(user)
+
+    mock_path = "taiga.projects.userstories.api.UserStoryViewSet.pre_conditions_on_save"
+    with patch(mock_path) as m:
+        url = reverse("userstories-list")
+        data = {"subject": "test",
+                      "project": project.id,
+                      "status": f.UserStoryStatusFactory.create(project=project).id}
+        response = client.json.post(url, json.dumps(data))
+        assert response.status_code == 201
+
+        userstory_id = json.loads(response.content)["id"]
+        url = reverse("userstories-detail", args=(userstory_id,))
+        data = {"version":1, "subject": "test 1"}
+        response = client.patch(url, json.dumps(data), content_type="application/json")
+        assert response.status_code == 200
+
+        data = {"version":2, "subject": "test 2"}
+        response = client.patch(url, json.dumps(data), content_type="application/json")
+        assert response.status_code == 200
+
+
+def test_valid_concurrent_save_for_us_different_fields(client):
+    user = f.UserFactory.create()
+    project = f.ProjectFactory.create(owner=user)
+    membership = f.MembershipFactory.create(project=project, user=user, is_owner=True)
+    client.login(user)
+
+    mock_path = "taiga.projects.userstories.api.UserStoryViewSet.pre_conditions_on_save"
+    with patch(mock_path) as m:
+        url = reverse("userstories-list")
+        data = {"subject": "test",
+                      "project": project.id,
+                      "status": f.UserStoryStatusFactory.create(project=project).id}
+        response = client.json.post(url, json.dumps(data))
+        assert response.status_code == 201
+
+        userstory_id = json.loads(response.content)["id"]
+        url = reverse("userstories-detail", args=(userstory_id,))
+        data = {"version":1, "subject": "test 1"}
+        response = client.patch(url, json.dumps(data), content_type="application/json")
+        assert response.status_code == 200
+
+        data = {"version":1, "description": "test 2"}
+        response = client.patch(url, json.dumps(data), content_type="application/json")
+        assert response.status_code == 200
+
 
 def test_invalid_concurrent_save_for_task(client):
     user = f.UserFactory.create()
     project = f.ProjectFactory.create(owner=user)
     membership = f.MembershipFactory.create(project=project, user=user, is_owner=True)
-    task = f.TaskFactory.create(version=10, project=project)
     client.login(user)
 
     mock_path = "taiga.projects.tasks.api.TaskViewSet.pre_conditions_on_save"
     with patch(mock_path) as m:
-        url = reverse("tasks-detail", args=(task.id,))
+        url = reverse("tasks-list")
-        data = {"version":9}
+        data = {"subject": "test",
+                      "project": project.id,
+                      "status": f.TaskStatusFactory.create(project=project).id}
+        response = client.json.post(url, json.dumps(data))
+        assert response.status_code == 201
+
+        task_id = json.loads(response.content)["id"]
+        url = reverse("tasks-detail", args=(task_id,))
+        data = {"version":1, "subject": "test 1"}
+        response = client.patch(url, json.dumps(data), content_type="application/json")
+        assert response.status_code == 200
+
+        data = {"version":1, "subject": "test 2"}
         response = client.patch(url, json.dumps(data), content_type="application/json")
         assert response.status_code == 400
 
-def test_valid_concurrent_save_for_task(client):
+
+def test_valid_concurrent_save_for_task_different_versions(client):
     user = f.UserFactory.create()
     project = f.ProjectFactory.create(owner=user)
     membership = f.MembershipFactory.create(project=project, user=user, is_owner=True)
-    task = f.TaskFactory.create(version=10, project=project)
     client.login(user)
 
     mock_path = "taiga.projects.tasks.api.TaskViewSet.pre_conditions_on_save"
     with patch(mock_path) as m:
-        url = reverse("tasks-detail", args=(task.id,))
+        url = reverse("tasks-list")
-        data = {"version": 10}
+        data = {"subject": "test",
+                      "project": project.id,
+                      "status": f.TaskStatusFactory.create(project=project).id}
+        response = client.json.post(url, json.dumps(data))
+        assert response.status_code == 201
+
+        task_id = json.loads(response.content)["id"]
+        url = reverse("tasks-detail", args=(task_id,))
+        data = {"version":1, "subject": "test 1"}
+        response = client.patch(url, json.dumps(data), content_type="application/json")
+        assert response.status_code == 200
+
+        data = {"version":2, "subject": "test 2"}
+        response = client.patch(url, json.dumps(data), content_type="application/json")
+        assert response.status_code == 200
+
+
+def test_valid_concurrent_save_for_task_different_fields(client):
+    user = f.UserFactory.create()
+    project = f.ProjectFactory.create(owner=user)
+    membership = f.MembershipFactory.create(project=project, user=user, is_owner=True)
+    client.login(user)
+
+    mock_path = "taiga.projects.tasks.api.TaskViewSet.pre_conditions_on_save"
+    with patch(mock_path) as m:
+        url = reverse("tasks-list")
+        data = {"subject": "test",
+                      "project": project.id,
+                      "status": f.TaskStatusFactory.create(project=project).id}
+        response = client.json.post(url, json.dumps(data))
+        assert response.status_code == 201
+
+        task_id = json.loads(response.content)["id"]
+        url = reverse("tasks-detail", args=(task_id,))
+        data = {"version":1, "subject": "test 1"}
+        response = client.patch(url, json.dumps(data), content_type="application/json")
+        assert response.status_code == 200
+
+        data = {"version":1, "description": "test 2"}
         response = client.patch(url, json.dumps(data), content_type="application/json")
-        assert json.loads(response.content)['version'] == 11
         assert response.status_code == 200
-        task = Task.objects.get(id=task.id)
-        assert task.version == 11