Purge data when delete tile #768

rodfersou · 2017-11-17T16:42:49Z

hvelarde

is looking good; we need an upgrade step and a clear changelog entry.

hvelarde · 2017-11-17T18:59:27Z

src/collective/cover/content.py

+        annotation_tiles = {
+            k.split('.')[-1]: k
+            for k in annotations.keys()
+            if k.startswith(u'plone.tiles.data') or


this should be written differently:

prefixes = ('plone.tiles.data', 'plone.tiles.configuration', 'plone.tiles.permission') ... for k in annotations.keys() if k.startswith(prefixes)

nice tip! thanks

hvelarde · 2017-11-17T19:02:03Z

src/collective/cover/content.py

+        for tile_id, annotation_key in annotation_tiles.iteritems():
+            if tile_id in layout_tiles:
+                continue
+            annotations.pop(annotation_key, None)


rename the variable to key and remove the annotation like this:

del annotations[key]

hvelarde · 2017-11-17T19:03:07Z

src/collective/cover/tests/test_basic_tile.robot

@@ -177,6 +177,17 @@ Test Basic Tile

    # go back to compose view and edit the tile
    Compose Cover
+
+    ${compose_url} =  Execute Javascript


let's move this test out of here to make it easier to understand.

hvelarde · 2017-11-17T19:06:12Z

src/collective/cover/tests/test_basic_tile.robot

@@ -186,6 +197,16 @@ Test Basic Tile
    # check for successful AJAX refresh
    Wait Until Page Contains  ${title_sample}

+    Go to  ${edit_url}


add a comment to document why you are doing this and what are you trying to demonstrate.

rodfersou · 2017-11-18T00:30:23Z

@hvelarde done

hvelarde · 2017-11-20T13:39:07Z

src/collective/cover/content.py

+           This method purge all tile annotation data for tiles where there are
+           no reference in layout.
+        """
+        prefixes = ('plone.tiles.data', 'plone.tiles.configuration', 'plone.tiles.permission')


these prefixes are defined as constants we should import and use the constants instead of hard-coding them.

hvelarde · 2017-11-20T13:41:03Z

src/collective/cover/static/js/layout_edit.js

@@ -290,41 +290,10 @@
          'top': '0',
          'width': '15px'
        });
-        elements = elements !== undefined ? elements : le.find('.' + column_class + ', .' + tile_class + ', .' + row_class);


this file is not in the JS registry, we don't need to cook the registry in the upgrade step; anyway, I think is a good idea to add it in the registry to guarantee it will be always updated in production sites.

Let's think about it when we rewrite code to use webpack; I'll remove the upgrade step to cook javascript now.

Put ?v=xxxx in the file call to avoid problems with caching.

Sorry. I saw you registered js.

hvelarde · 2017-11-20T13:41:59Z

src/collective/cover/tests/test_base_tile.py

@@ -66,24 +65,3 @@ def test_default_configuration(self):

    def test_accepted_content_types(self):
        self.assertEqual(self.tile.accepted_ct(), ALL_CONTENT_TYPES)
-
-    def test_delete_tile_persistent_data(self):


I would like to keep this test to guarantee all persistent data, conffiguration an permissions are removed at low level and the ObjectModifiedEvent is being fired.

This test uses the tile.delete() method that was suggested to be removed.

we have to think on how to test the feature on low level.

hvelarde · 2017-11-20T14:00:14Z

src/collective/cover/content.py

+        """
+        prefixes = ('plone.tiles.data', 'plone.tiles.configuration', 'plone.tiles.permission')
+        layout_tiles = self.list_tiles()
+        annotations = IAnnotations(self)


this has to be divided in a two-step process: on plone.tiles persistent tiles data can be stored on a storage different of annotations, that means it's safer to use the data manager API to remove the data from the tile; our annotations (configuration and permission) can be removed directly, but we will need to test them at least on the base tile at low level to guarantee it's working.

it's probably better to keep the API also on configuration and permission, so we could call a delete() method to do the work.

I don't understand what you mean.. storage variable is an alias to annotation property

I mean you have to delete the annotation using the tile manager.

didn't work, tile collection test breaks because I don't know the type of the deleted tile to instantiate the correct type..

hvelarde · 2017-11-20T14:04:41Z

@idgserpro @claytonc I need more eyes here, please.

hvelarde · 2017-11-20T15:52:44Z

src/collective/cover/config.py

@@ -37,3 +37,5 @@
 # In case that no value was set to that field
 # we need to detect it and use it
 DEFAULT_SEQUENCEWIDGET_VALUE = '--NOVALUE--'
+
+ANNOTATION_PREFIXES = ('plone.tiles.data', 'plone.tiles.configuration', 'plone.tiles.permission')


these constants are defined somewhere else:

https://github.com/collective/collective.cover/blob/1.6b4/src/collective/cover/tiles/configuration.py#L17

https://github.com/collective/collective.cover/blob/1.6b4/src/collective/cover/tiles/permissions.py#L9

https://github.com/plone/plone.tiles/blob/2.0.0b3/plone/tiles/data.py#L37

@rodfersou did you saw this?

Yes, I saw, need to review this

hvelarde · 2017-11-20T15:54:30Z

src/collective/cover/tests/test_base_tile.py

@@ -66,24 +65,3 @@ def test_default_configuration(self):

    def test_accepted_content_types(self):
        self.assertEqual(self.tile.accepted_ct(), ALL_CONTENT_TYPES)
-
-    def test_delete_tile_persistent_data(self):


we have to think on how to test the feature on low level.

hvelarde · 2017-11-20T15:56:30Z

src/collective/cover/content.py

+        """
+        prefixes = ('plone.tiles.data', 'plone.tiles.configuration', 'plone.tiles.permission')
+        layout_tiles = self.list_tiles()
+        annotations = IAnnotations(self)


I mean you have to delete the annotation using the tile manager.

hvelarde · 2017-11-20T15:58:28Z

src/collective/cover/tiles/base.py

-        """
-        logger.debug('Deleting tile {0}'.format(self.id))
-
-        data_mgr = ITileDataManager(self)


this is what you need to do in the new purge_deleted_tiles() method.

idgserpro · 2017-11-20T20:07:10Z

src/collective/cover/tests/test_purge_data.robot

+    ${compose_url} =  Execute Javascript
+    ...  return (function() {
+    ...    return location.href;
+    ...  })();


${compose_url} = Get Location

idgserpro · 2017-11-20T20:09:17Z

src/collective/cover/tests/test_purge_data.robot

+    # confirm that data is still there
+    Go to  ${edit_url}
+    ${value} =  Get value  ${title_field_id}
+    Should be equal  ${title_sample}  ${value}


Is it even necessary to test this? You just clicked Save.

idgserpro · 2017-11-20T20:12:38Z

src/collective/cover/tests/test_purge_data.robot

+    # confirm that data is gone
+    Go to  ${edit_url}
+    ${value} =  Get value  ${title_field_id}
+    Should Not be equal  ${title_sample}  ${value}


In fact, it needs to be empty. They are different things.

idgserpro · 2017-11-20T20:19:21Z

src/collective/cover/tests/test_purge_data.robot

+    ...  return (function() {
+    ...    var $link = jQuery('${edit_link_selector}');
+    ...    return $link.attr('href');
+    ...  })();


Get Element Attribute:

http://robotframework.org/Selenium2Library/Selenium2Library.html#Get%20Element%20Attribute

idgserpro · 2017-11-20T20:23:15Z

src/collective/cover/tests/test_upgrades.py

+        self._do_upgrade_step(step)
+
+        with self.assertRaises(KeyError):
+            annotations[key]


If annotations have the keys method:

self.assertNotIn(key, annotations.keys())

in Python 3 there is no keys() method; we must use self.assertNotIn(key, annotations) instead.

rodfersou · 2017-11-20T22:31:51Z

src/collective/cover/content.py

+
+           To delete tile we just need the ID, the type can be anything.
+        """
+        tile = self.restrictedTraverse('foo/' + tile_id)


@datakurre when I try to instantiate a deleted tile (to delete it's data) I get this exception:

ipdb> self.restrictedTraverse(u'foo/' + tile_id) *** NotFound: f ipdb> self.restrictedTraverse(u'collective.cover.base/' + tile_id) *** NotFound: c ipdb> self.restrictedTraverse(u'collective.cover.basic/' + tile_id) *** NotFound: c

Can you help me please understand why this code don't work, while it is almost the same code from get_tile method?

hvelarde · 2017-11-21T16:46:39Z

I'm going to merge this after Travis; if somebody else has something to say, it's now.

idgserpro · 2017-11-21T23:25:45Z

src/collective/cover/tests/test_base_tile.py

+        from collective.cover.config import PERMISSIONS_PREFIX
+        from zope.annotation import IAnnotations
+        from zope.component import eventtesting
+        from zope.lifecycleevent import IObjectModifiedEvent


@hvelarde why in tests do you import inside the methods?

because is easier to maintain later.

rodfersou force-pushed the issue_765 branch 2 times, most recently from a9d3a0f to 1116c00 Compare November 17, 2017 18:41

hvelarde requested changes Nov 17, 2017

View reviewed changes

hvelarde requested review from fredvd, claytonc and idgserpro November 17, 2017 19:07

hvelarde added the in progress label Nov 17, 2017

rodfersou force-pushed the issue_765 branch from 1295212 to 45bbbe1 Compare November 17, 2017 21:12

rodfersou added ready and removed in progress labels Nov 18, 2017

hvelarde requested changes Nov 20, 2017

View reviewed changes

hvelarde reviewed Nov 20, 2017

View reviewed changes

hvelarde requested changes Nov 20, 2017

View reviewed changes

rodfersou force-pushed the issue_765 branch 7 times, most recently from 2833949 to 7e277a3 Compare November 20, 2017 20:01

idgserpro reviewed Nov 20, 2017

View reviewed changes

rodfersou commented Nov 20, 2017

View reviewed changes

rodfersou force-pushed the issue_765 branch 6 times, most recently from e162665 to ee28d94 Compare November 20, 2017 23:56

rodfersou force-pushed the issue_765 branch 3 times, most recently from db42a9b to facb577 Compare November 21, 2017 00:02

rodfersou added 9 commits November 20, 2017 22:04

Add test to demonstrate issue

36dfc3b

Fix view DeleteTile call

9040b0d

Remove DeleteTile view

e61e831

Add code to purge unused annotations

09f43bd

Add upgrade step

f83f88c

Update changelog

318a8ef

Move annotation prefixes constant to config.py file

ccefd8e

Register layout_edit.js

facb577

Review delete tile tests

40ab3cd

rodfersou force-pushed the issue_765 branch from c512129 to e67c9d5 Compare November 21, 2017 13:44

rodfersou and others added 2 commits November 21, 2017 11:52

Fix test

e67c9d5

Refactor

4495074

hvelarde approved these changes Nov 21, 2017

View reviewed changes

hvelarde merged commit ba54a2a into master Nov 21, 2017

hvelarde deleted the issue_765 branch November 21, 2017 19:16

idgserpro reviewed Nov 21, 2017

View reviewed changes

Purge data when delete tile #768

Purge data when delete tile #768

Conversation

rodfersou commented Nov 17, 2017 • edited Loading

hvelarde left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rodfersou commented Nov 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hvelarde Nov 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hvelarde commented Nov 20, 2017 • edited Loading

Choose a reason for hiding this comment

hvelarde Nov 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rodfersou Nov 20, 2017 • edited Loading

Choose a reason for hiding this comment

hvelarde commented Nov 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rodfersou commented Nov 17, 2017 •

edited

Loading

hvelarde Nov 20, 2017 •

edited

Loading

hvelarde commented Nov 20, 2017 •

edited

Loading

hvelarde Nov 20, 2017 •

edited

Loading

rodfersou Nov 20, 2017 •

edited

Loading