Move the `Comment` class to the new backend interface #2225

sphuber · 2018-11-21T08:13:11Z

coveralls · 2018-11-21T08:33:40Z

Coverage increased (+0.3%) to 69.052% when pulling f5406dd on sphuber:fix_2223_migrate_comment_backend_interface into eaee1cc on aiidateam:provenance_redesign.

muhrin

Do we have any tests? It looks like your Comment class was abstract but this doesn't seem to have been caught. Maybe everywhere in the code is still uses backend.comments....if so these should be changed to Comment.

muhrin · 2018-11-21T08:18:52Z

aiida/orm/comments.py

+        """
+        return self._backend_entity.is_stored()
+
+    @abc.abstractproperty


Suggested change

@abc.abstractproperty

muhrin · 2018-11-21T08:19:58Z

aiida/orm/comments.py

+
+    @abc.abstractproperty
+    def uuid(self):
+        pass


Suggested change

pass

return self._backend_entity.uuid

muhrin · 2018-11-21T08:21:24Z

aiida/orm/comments.py

+
+    @abc.abstractmethod
+    def get_ctime(self):
+        pass


Suggested change

pass

return self._backend_entity.get_ctime()

muhrin · 2018-11-21T08:23:26Z

aiida/orm/comments.py

+
+    @abc.abstractmethod
+    def get_user(self):
+        pass


Suggested change

pass

return users.User.from_backend_entity(self._backend_entity.get_user())

muhrin · 2018-11-21T08:23:51Z

aiida/orm/comments.py

+
+    @abc.abstractmethod
+    def set_user(self, value):
+        pass


Suggested change

pass

self._backend_entity.set_user(value.backend_entity)

muhrin · 2018-11-21T08:30:51Z

aiida/orm/implementation/django/comment.py


    def get_ctime(self):
-        return self.dbcomment.ctime
+        return self._dbmodel.ctime

    def set_ctime(self, val):


See comment above about ctime

muhrin · 2018-11-21T08:31:53Z

aiida/orm/implementation/sqlalchemy/comment.py

+        """
+        super(SqlaComment, self).__init__(backend)
+        type_check(user, users.SqlaUser)
+        self._dbmodel = utils.ModelWrapper(DbComment(node=node.dbnode, user=user.dbmodel, content=content))

    @property
    def pk(self):


Delete (in superclass)

muhrin · 2018-11-21T08:31:59Z

aiida/orm/implementation/sqlalchemy/comment.py


    @property
    def pk(self):
-        return self.dbcomment.id
+        return self._dbmodel.pk

    @property
    def id(self):


Delete (in superclass)

muhrin · 2018-11-21T08:32:10Z

aiida/orm/implementation/sqlalchemy/comment.py


    @property
-    def to_be_stored(self):
-        return self.dbcomment.id is None
+    def stored(self):


-> is_stored()

muhrin · 2018-11-21T08:32:24Z

aiida/orm/implementation/sqlalchemy/comment.py


    def get_ctime(self):
-        return self.dbcomment.ctime
+        return self._dbmodel.ctime

    def set_ctime(self, val):


consider deleting

sphuber · 2018-11-21T08:36:25Z

The only "tests" that were in place are indirect through the verdi comment command. These are passing, but the coverage is shit and so it doesn't say much. I will see if I can add some proper unit tests for the ORM classes themselves

giovannipizzi · 2018-11-21T09:23:48Z

Quick comment - I was just wondering if we need an entity also for comments (and similarly for links) or these can just be considered as a "property" of Node (even if in our implementation they live in a different DB table). Because then intuitively if it's an entity one would expect to be able to "join" them via the query builder. Of course since this implemented it's fine to keep as it is, I point out this just for discussion and to decide what to do with the other tables that do not have a corresponding entity (notably links but maybe something else?)

muhrin · 2018-11-21T09:37:07Z

Good point @giovannipizzi , so long as the interface is clear (i.e. the comment interaction is properly defined through the Node interface) then I think it's fine (and maybe even desirable) to not expose this as a first class orm entity.

sphuber · 2018-11-21T09:41:03Z

At the current stage having it as a separate full-fledged entity seems a bit overkill, agree, but I can easily imagine that when we do start to use it, it will be very useful to have it queryable. As I understand its original intention, it is for users to comment on certain nodes, i.e. in a discussion on a platform like MaterialsCloud or just when sharing nodes. For this use case, being able to find all nodes on which a particular user has commented, would be useful. Should we leave it like this for now and I just add tests for the ORM classes?

muhrin · 2018-11-21T09:47:40Z

Yes, leave it as is. Some testing is good but for now I reckon a 'system testing' approach is fine i.e. just rely on higher level functions like the verdi command line tests to put it through its paces.

giovannipizzi · 2018-11-21T10:31:41Z

Ok for me.
Still, I think we should have at the next meeting a discussion on what should be an Entity and what should be accessible only via Node methods or the QueryBuilder (and yes, they should in the future be queryable via the QB, but I find it more intuitive to do

qb.append(Node, filters={'extras': ..., 'comments': ...})

rather

qb.append(Node, tag='n')
qb.append(Comment, with_node='n', filters=...)

even if maybe it makes the QB code more complex. I'm open to suggestions.
Also mentioning @lekah for comments

sphuber · 2018-11-21T16:44:22Z

I added support for querying Comment to the QueryBuilder which allowed to clean up the methods on Node that deal with comments and made them backend independent.

muhrin · 2018-11-22T08:12:34Z

aiida/orm/implementation/django/comment.py

-            self.dbcomment.save()
+    def set_mtime(self, value):
+        self._dbmodel.mtime = value
+        if self.is_stored:


This is not needed. The ModelWrapper does this for all attributes anyway. Sorry I missed it the first time.

muhrin · 2018-11-22T08:13:09Z

aiida/orm/implementation/sqlalchemy/comment.py

-            self.dbcomment.save()
+    def set_user(self, value):
+        self._dbmodel.user = value
+        if self.is_stored:


muhrin · 2018-11-22T08:15:23Z

aiida/orm/implementation/django/comment.py

+    def store(self):
+        """Can only store if both the node and user are stored as well."""
+        if self._dbmodel.dbnode.id is None or self._dbmodel.user.id is None:
+            raise exceptions.ModificationNotAllowed


Suggested change

raise exceptions.ModificationNotAllowed

raise exceptions.ModificationNotAllowed('The corresponding node and/or user are not stored')

Balls, you're breaking 'em

muhrin · 2018-11-22T08:16:04Z

aiida/orm/implementation/sqlalchemy/comment.py

+        """Can only store if both the node and user are stored as well."""
+        if self._dbmodel.dbnode.id is None or self._dbmodel.user.id is None:
+            self._dbmodel.dbnode = None
+            raise exceptions.ModificationNotAllowed


Suggested change

raise exceptions.ModificationNotAllowed

raise exceptions.ModificationNotAllowed('The corresponding node and/or user are not stored')

muhrin · 2018-11-22T08:23:04Z

aiida/orm/__init__.py

           calculation.__all__ +
+           comments.__all__ +


Suggested change

comments.__all__ +

muhrin · 2018-11-22T10:16:25Z

aiida/orm/implementation/django/comment.py

+    def store(self):
+        """Can only store if both the node and user are stored as well."""
+        if self._dbmodel.dbnode.id is None or self._dbmodel.user.id is None:
+            raise exceptions.ModificationNotAllowed


Balls, you're breaking 'em

This allows to make the methods of `Node` backend independent and as a result makes the implementation a lot simpler. The command line interface is also adapted and simplified.

sphuber requested a review from muhrin November 21, 2018 08:13

muhrin suggested changes Nov 21, 2018

View reviewed changes

sphuber force-pushed the fix_2223_migrate_comment_backend_interface branch 2 times, most recently from ea4cc22 to 10eda70 Compare November 21, 2018 16:39

sphuber force-pushed the fix_2223_migrate_comment_backend_interface branch 2 times, most recently from 7e9c96e to 64ad5e1 Compare November 21, 2018 18:27

muhrin suggested changes Nov 22, 2018

View reviewed changes

aiida/orm/__init__.py Outdated

calculation.__all__ +

comments.__all__ +

Copy link

Contributor

muhrin Nov 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

comments.__all__ +

Move the Comment class to the new backend interface

84a5756

sphuber force-pushed the fix_2223_migrate_comment_backend_interface branch from 64ad5e1 to ff09cde Compare November 22, 2018 09:56

muhrin suggested changes Nov 22, 2018

View reviewed changes

Add support for Comment ORM class to QueryBuilder

f5406dd

This allows to make the methods of `Node` backend independent and as a result makes the implementation a lot simpler. The command line interface is also adapted and simplified.

sphuber force-pushed the fix_2223_migrate_comment_backend_interface branch from ff09cde to f5406dd Compare November 22, 2018 10:23

muhrin approved these changes Nov 22, 2018

View reviewed changes

sphuber merged commit 9501779 into aiidateam:provenance_redesign Nov 22, 2018

sphuber deleted the fix_2223_migrate_comment_backend_interface branch November 22, 2018 11:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move the `Comment` class to the new backend interface #2225

Move the `Comment` class to the new backend interface #2225

sphuber commented Nov 21, 2018

coveralls commented Nov 21, 2018 •

edited

Loading

muhrin left a comment

muhrin Nov 21, 2018

muhrin Nov 21, 2018

muhrin Nov 21, 2018

muhrin Nov 21, 2018

muhrin Nov 21, 2018

muhrin Nov 21, 2018

muhrin Nov 21, 2018

muhrin Nov 21, 2018

muhrin Nov 21, 2018

muhrin Nov 21, 2018

sphuber commented Nov 21, 2018

giovannipizzi commented Nov 21, 2018

muhrin commented Nov 21, 2018

sphuber commented Nov 21, 2018

muhrin commented Nov 21, 2018

giovannipizzi commented Nov 21, 2018

sphuber commented Nov 21, 2018

muhrin Nov 22, 2018

muhrin Nov 22, 2018

muhrin Nov 22, 2018

muhrin Nov 22, 2018

muhrin Nov 22, 2018

muhrin Nov 22, 2018

muhrin Nov 22, 2018

	pass
	return users.User.from_backend_entity(self._backend_entity.get_user())

	raise exceptions.ModificationNotAllowed
	raise exceptions.ModificationNotAllowed('The corresponding node and/or user are not stored')

Move the Comment class to the new backend interface #2225

Move the Comment class to the new backend interface #2225

Conversation

sphuber commented Nov 21, 2018

coveralls commented Nov 21, 2018 • edited Loading

muhrin 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

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

sphuber commented Nov 21, 2018

giovannipizzi commented Nov 21, 2018

muhrin commented Nov 21, 2018

sphuber commented Nov 21, 2018

muhrin commented Nov 21, 2018

giovannipizzi commented Nov 21, 2018

sphuber commented Nov 21, 2018

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

Move the `Comment` class to the new backend interface #2225

Move the `Comment` class to the new backend interface #2225

coveralls commented Nov 21, 2018 •

edited

Loading