-
Notifications
You must be signed in to change notification settings - Fork 80
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Study and StudyPerson object #70
Conversation
Alright, this should be good to merge if all tests pass. |
@staticmethod | ||
def create(owner): | ||
def _status_setter_checks(self, conn_handler): | ||
r"""Perform any extra checks that needed to be done before setting the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should probably change the docstring, as here is using the one from the base class.
A few more comments @squirrelo |
# insert efo information into database | ||
sql = ("INSERT INTO qiita.{0}_experimental_factor (study_id, " | ||
"efo_id) VALUES (%s, %s)".format(cls._table)) | ||
conn_handler.executemany(sql, [(study_id, e) for e in efo]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if the user passes an empty list?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now both raise IncompetentQiitaDeveloperError because you need at least one EFO attached to the study.
@squirrelo just a small comment. Can you also add a specific test case for it? |
""" | ||
raise QiitaDBNotImplementedError() | ||
if efo_vals == []: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if not efo_vals
👍 given the tests pass. There is a lot of code here, so it would be awesome if somebody else can do a quick review and merge!! |
@@ -218,10 +218,10 @@ def status(self, status): | |||
self._check_subclass() | |||
|
|||
# Perform any extra checks needed before we update the status in the DB | |||
self._status_setter_checks() | |||
conn_handler = SQLConnectionHandler() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is strange, conn_handler is a parameter but the first thing it does is conn_handler = SQLConnectionHandler()
A couple of comments, after that it should be ok to merge. |
👍 for the great jif. |
Closes #41 #55
Take two: Finally the big thing that all the tiny pull requests were for.
This is the study object itself, as well as the study person object that holds info on the people involved in a study. Please check this over and let me know if I missed any wanted functionality as well as the usual code stuff.
Also, the delete functionality is purposely not implemented currently. It will be hairy to do and not worth the time right now as the other objects need development an we aren't really deleting anything right now.