Merge pull request #555 from inkhey/fix/535/Do_not_create_empty_revision

tracim/tracim/controllers/content.py

@@ -174,7 +174,7 @@ class UserWorkspaceFolderFileRestController(TIMWorkspaceContentRestController):
 
     @property
     def _err_url(self):
-        return tg.url('/dashboard/workspaces/{}/folders/{}/file/{}')
+        return tg.url('/workspaces/{}/folders/{}/files/{}')
 
     @property
     def _item_type(self):
@@ -376,19 +376,13 @@ class UserWorkspaceFolderFileRestController(TIMWorkspaceContentRestController):
         try:
             api = ContentApi(tmpl_context.current_user)
             item = api.get_one(int(item_id), self._item_type, workspace)
-            label_changed = False
-            if label is not None and label != item.label:
-                label_changed = True
-
-            if label is None:
-                label = ''
-
-            # TODO - D.A. - 2015-03-19
-            # refactor this method in order to make code easier to understand
 
             with new_revision(item):
+                if label:
+                    # This case is the default "file title and description
+                    # update" In this case the file itself is not revisionned
 
-                if (comment and label) or (not comment and label_changed):
+                    # Update description and label
                     updated_item = api.update_content(
                         item, label if label else item.label,
                         comment if comment else ''
@@ -401,31 +395,21 @@ class UserWorkspaceFolderFileRestController(TIMWorkspaceContentRestController):
                         return render_invalid_integrity_chosen_path(
                             updated_item.get_label_as_file(),
                         )
-
                     api.save(updated_item, ActionDescription.EDITION)
-
-                    # This case is the default "file title and description
-                    # update" In this case the file itself is not revisionned
-
                 else:
                     # So, now we may have a comment and/or a file revision
-                    if comment and '' == label:
-                        comment_item = api.create_comment(workspace,
-                                                          item, comment,
-                                                          do_save=False)
-
-                        if not isinstance(file_data, FieldStorage):
-                            api.save(comment_item, ActionDescription.COMMENT)
-                        else:
-                            # The notification is only sent
-                            # if the file is NOT updated
-                            #
-                            # If the file is also updated,
-                            # then a 'file revision' notification will be sent.
-                            api.save(comment_item,
-                                     ActionDescription.COMMENT,
-                                     do_notify=False)
+                    comment_item = None
+                    file_revision = None
 
+                    # INFO - G.M - 20/03/2018 - Add new comment
+                    if comment:
+                        comment_item = api.create_comment(
+                            workspace,
+                            item,
+                            comment,
+                            do_save=False
+                        )
+                    # INFO - G.M - 20/03/2018 - Add new file-revision
                     if isinstance(file_data, FieldStorage):
                         api.update_file_data(item,
                                              file_data.filename,
@@ -440,8 +424,20 @@ class UserWorkspaceFolderFileRestController(TIMWorkspaceContentRestController):
                             return render_invalid_integrity_chosen_path(
                                 item.get_label_as_file(),
                             )
-
+                        file_revision = True
+
+                    # INFO - G.M - 20/03/2018 - Save revision/comment
+                    if comment_item and file_revision:
+                        api.save(
+                            comment_item,
+                            ActionDescription.COMMENT,
+                            do_notify= False
+                        )
+                        api.save(item, ActionDescription.REVISION)
+                    elif file_revision:
                         api.save(item, ActionDescription.REVISION)
+                    elif comment_item:
+                        api.save(comment_item, ActionDescription.COMMENT)
 
             msg = _('{} updated').format(self._item_type_label)
             tg.flash(msg, CST.STATUS_OK)
@@ -449,6 +445,14 @@ class UserWorkspaceFolderFileRestController(TIMWorkspaceContentRestController):
                                              tmpl_context.folder_id,
                                              item.content_id))
 
+        except SameValueError:
+            not_updated = '{} not updated: the content did not change'
+            msg = _(not_updated).format(self._item_type_label)
+            tg.flash(msg, CST.STATUS_WARNING)
+            tg.redirect(self._err_url.format(tmpl_context.workspace_id,
+                                             tmpl_context.folder_id,
+                                             item_id))
+
         except ValueError as e:
             error = '{} not updated - error: {}'
             msg = _(error).format(self._item_type_label,
@@ -459,6 +463,7 @@ class UserWorkspaceFolderFileRestController(TIMWorkspaceContentRestController):
                                              item_id))
 
 
+
 class UserWorkspaceFolderPageRestController(TIMWorkspaceContentRestController):
     """
     manage a path like this: /workspaces/1/folders/XXX/pages/4
@@ -577,9 +582,11 @@ class UserWorkspaceFolderPageRestController(TIMWorkspaceContentRestController):
 
         tg.flash(_('Page created'), CST.STATUS_OK)
         redirect = '/workspaces/{}/folders/{}/pages/{}'
-        tg.redirect(tg.url(redirect).format(tmpl_context.workspace_id,
-                                            tmpl_context.folder_id,
-                                            page.content_id))
+        tg.redirect(tg.url(redirect).format(
+            tmpl_context.workspace_id,
+            tmpl_context.folder_id,
+            page.content_id)
+        )
 
     @tg.require(current_user_is_contributor())
     @tg.expose()
@@ -606,21 +613,28 @@ class UserWorkspaceFolderPageRestController(TIMWorkspaceContentRestController):
             tg.redirect(self._std_url.format(tmpl_context.workspace_id,
                                              tmpl_context.folder_id,
                                              item.content_id))
-        except SameValueError as e:
+        except SameValueError:
             not_updated = '{} not updated: the content did not change'
             msg = _(not_updated).format(self._item_type_label)
             tg.flash(msg, CST.STATUS_WARNING)
-            tg.redirect(self._err_url.format(tmpl_context.workspace_id,
-                                             tmpl_context.folder_id,
-                                             item_id))
+            tg.redirect(
+                self._err_url.format(
+                    tmpl_context.workspace_id,
+                    tmpl_context.folder_id,
+                    item_id
+                )
+            )
         except ValueError as e:
             not_updated = '{} not updated - error: {}'
             msg = _(not_updated).format(self._item_type_label, str(e))
             tg.flash(msg, CST.STATUS_ERROR)
-            tg.redirect(self._err_url.format(tmpl_context.workspace_id,
-                                             tmpl_context.folder_id,
-                                             item_id))
-
+            tg.redirect(
+                self._err_url.format(
+                    tmpl_context.workspace_id,
+                    tmpl_context.folder_id,
+                    item_id
+                )
+            )
 
 class UserWorkspaceFolderThreadRestController(TIMWorkspaceContentRestController):
     """

tracim/tracim/lib/content.py

@@ -920,7 +920,9 @@ class ContentApi(object):
 
     def update_content(self, item: Content, new_label: str, new_content: str=None) -> Content:
         if item.label==new_label and item.description==new_content:
-            raise SameValueError(_('The content did not changed'))
+            # TODO - G.M - 20-03-2018 - Fix internatization for webdav access.
+            # Internatization disabled in libcontent for now.
+            raise SameValueError('The content did not changed')
         item.owner = self._user
         item.label = new_label
         item.description = new_content if new_content else item.description # TODO: convert urls into links
@@ -928,6 +930,9 @@ class ContentApi(object):
         return item
 
     def update_file_data(self, item: Content, new_filename: str, new_mimetype: str, new_content: bytes) -> Content:
+        if new_mimetype == item.file_mimetype and \
+                new_content == item.depot_file.file.read():
+            raise SameValueError('The content did not changed')
         item.owner = self._user
         item.file_name = new_filename
         item.file_mimetype = new_mimetype

tracim/tracim/lib/webdav/__init__.py

@@ -5,11 +5,15 @@ from wsgidav import util
 from wsgidav import compat
 
 from tracim.lib.content import ContentApi
+from tracim.lib.utils import SameValueError
+from tracim.lib.base import logger
 from tracim.model import new_revision
 from tracim.model.data import ActionDescription
 from tracim.model.data import ContentType
 from tracim.model.data import Content
 from tracim.model.data import Workspace
+from wsgidav.dav_error import DAVError
+from wsgidav.dav_error import HTTP_FORBIDDEN
 
 
 class HistoryType(object):
@@ -96,12 +100,33 @@ class FakeFileStream(object):
 
         self._file_stream.seek(0)
 
-        if self._content is None:
-            self.create_file()
-        else:
-            self.update_file()
-
-        transaction.commit()
+        try:
+            if self._content is None:
+                self.create_file()
+            else:
+                self.update_file()
+            transaction.commit()
+        except SameValueError:
+            # INFO - G.M - 21-03-2018 - Do nothing, file as not change.
+            msg = 'File {filename} modified through webdav did not change, transaction aborted.'  # nopep8
+            logger.debug(
+                self,
+                msg.format(filename=self._file_name)
+            )
+            transaction.abort()
+            transaction.begin()
+        except ValueError as e:
+            msg = 'File {filename} modified through webdav can\'t be updated: {exception}'  # nopep8
+            logger.debug(
+                self,
+                msg.format(
+                    filename=self._file_name,
+                    e=e.__str__,
+                )
+            )
+            transaction.abort()
+            transaction.begin()
+            raise DAVError(HTTP_FORBIDDEN)
 
     def create_file(self):
         """
@@ -138,5 +163,4 @@ class FakeFileStream(object):
                 util.guessMimeType(self._content.file_name),
                 self._file_stream.read()
             )
-
             self._api.save(self._content, ActionDescription.REVISION)

tracim/tracim/model/__init__.py

@@ -8,7 +8,8 @@ from sqlalchemy.orm.unitofwork import UOWTransaction
 from zope.sqlalchemy import ZopeTransactionExtension
 
 from tracim.lib.exception import ContentRevisionUpdateError, ContentRevisionDeleteError
-
+from tracim.lib.utils import SameValueError
+import transaction
 
 class RevisionsIntegrity(object):
     """
@@ -136,5 +137,12 @@ def new_revision(
                 content.new_revision()
             RevisionsIntegrity.add_to_updatable(content.revision)
             yield content
+        except SameValueError or ValueError as e:
+            # INFO - 20-03-2018 - renew transaction when error happened
+            # This avoid bad session data like new "temporary" revision
+            # to be add when problem happen.
+            transaction.abort()
+            transaction.begin()
+            raise e
         finally:
             RevisionsIntegrity.remove_from_updatable(content.revision)

tracim/tracim/tests/library/test_content_api.py

@@ -11,6 +11,7 @@ from tracim.lib.content import compare_content_for_sorting_by_type_and_name
 from tracim.lib.content import ContentApi
 from tracim.lib.group import GroupApi
 from tracim.lib.user import UserApi
+from tracim.lib.utils import SameValueError
 from tracim.lib.workspace import RoleApi
 from tracim.lib.workspace import WorkspaceApi
 from tracim.model import DBSession, new_revision, User
@@ -829,6 +830,60 @@ class TestContentApi(BaseTest, TestStandard):
         eq_('new content', updated.description)
         eq_(ActionDescription.EDITION, updated.revision_type)
 
+    @raises(SameValueError)
+    def test_update_no_change(self):
+        uapi = UserApi(None)
+        groups = [
+            GroupApi(None).get_one(Group.TIM_USER),
+            GroupApi(None).get_one(Group.TIM_MANAGER),
+            GroupApi(None).get_one(Group.TIM_ADMIN)
+        ]
+
+        user1 = uapi.create_user(
+            email='this.is@user',
+            groups=groups,
+            save_now=True,
+        )
+
+        workspace = WorkspaceApi(user1).create_workspace(
+            'test workspace',
+            save_now=True
+        )
+
+        user2 = uapi.create_user()
+        user2.email = 'this.is@another.user'
+        uapi.save(user2)
+
+        RoleApi(user1).create_one(
+            user2,
+            workspace,
+            UserRoleInWorkspace.CONTENT_MANAGER,
+            with_notif=False,
+            flush=True
+        )
+        api = ContentApi(user1)
+        with DBSession.no_autoflush:
+            page = api.create(
+                content_type=ContentType.Page,
+                workspace=workspace,
+                label="same_content",
+                do_save=False
+            )
+            page.description = "Same_content_here"
+        api.save(page, ActionDescription.CREATION, do_notify=True)
+        transaction.commit()
+
+        api2 = ContentApi(user2)
+        content2 = api2.get_one(page.content_id, ContentType.Any, workspace)
+        with new_revision(content2):
+            api2.update_content(
+                item=content2,
+                new_label='same_content',
+                new_content='Same_content_here'
+            )
+        api2.save(content2)
+        transaction.commit()
+
     def test_update_file_data(self):
         uapi = UserApi(None)
         groups = [GroupApi(None).get_one(Group.TIM_USER),
@@ -896,6 +951,67 @@ class TestContentApi(BaseTest, TestStandard):
         eq_(b'<html>hello world</html>', updated.depot_file.file.read())
         eq_(ActionDescription.REVISION, updated.revision_type)
 
+    @raises(SameValueError)
+    def test_update_no_change(self):
+        uapi = UserApi(None)
+        groups = [
+            GroupApi(None).get_one(Group.TIM_USER),
+            GroupApi(None).get_one(Group.TIM_MANAGER),
+            GroupApi(None).get_one(Group.TIM_ADMIN)
+        ]
+
+        user1 = uapi.create_user(
+            email='this.is@user',
+            groups=groups,
+            save_now=True,
+        )
+
+        workspace = WorkspaceApi(user1).create_workspace(
+            'test workspace',
+            save_now=True
+        )
+
+        user2 = uapi.create_user()
+        user2.email = 'this.is@another.user'
+        uapi.save(user2)
+
+        RoleApi(user1).create_one(
+            user2,
+            workspace,
+            UserRoleInWorkspace.CONTENT_MANAGER,
+            with_notif=False,
+            flush=True
+        )
+        api = ContentApi(user1)
+        with DBSession.no_autoflush:
+            page = api.create(
+                content_type=ContentType.Page,
+                workspace=workspace,
+                label="same_content",
+                do_save=False
+            )
+            api.update_file_data(
+                page,
+                'index.html',
+                'text/html',
+                b'<html>Same Content Here</html>'
+            )
+        api.save(page, ActionDescription.CREATION, do_notify=True)
+        transaction.commit()
+
+        api2 = ContentApi(user2)
+        content2 = api2.get_one(page.content_id, ContentType.Any, workspace)
+        with new_revision(content2):
+            api2.update_file_data(
+                page,
+                'index.html',
+                'text/html',
+                b'<html>Same Content Here</html>'
+            )
+        api2.save(content2)
+        transaction.commit()
+
+
     def test_archive_unarchive(self):
         uapi = UserApi(None)
         groups = [GroupApi(None).get_one(Group.TIM_USER),