Skip to content
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

Adding tags #2088

Merged
merged 7 commits into from
Mar 27, 2017
Merged

Adding tags #2088

merged 7 commits into from
Mar 27, 2017

Conversation

antgonza
Copy link
Member

Based on reviews during the Qiita meeting, I'm changed the way this feature works. You can see how it works in the attachment below. Another big change from original definition is that now we are using REST calls to get the tags and the updates.

Note that this adds the possibility of adding tags to studies and keeping track of them by user. However, we are still not showing or filtering the studies in the study list by this. I'll make another PR adding that functionality as this PR is pretty long (but not terrible) already.

tags

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 91.613% when pulling 0af93ff on antgonza:adding-tags into 2795046 on biocore:master.

@antgonza
Copy link
Member Author

@biocore/qiita-admin, wanna take a look/review?

Copy link
Contributor

@josenavas josenavas left a comment

Choose a reason for hiding this comment

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

Minor comments

@@ -1152,6 +1137,75 @@ def unshare(self, user):
qdb.sql_connection.TRN.add(sql, [self._id, user.id])
qdb.sql_connection.TRN.execute()

def add_tags(self, user, tags):
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be named update_tags, given that it can add and remove tags?

Copy link
Member Author

Choose a reason for hiding this comment

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

k

Parameters
----------
user: User object
The user to unshare the study with
Copy link
Contributor

Choose a reason for hiding this comment

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

Update doc

Copy link
Member Author

Choose a reason for hiding this comment

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

k

user: User object
The user to unshare the study with
tags : list of str
The tags to add to the study
Copy link
Contributor

Choose a reason for hiding this comment

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

The tags to attach to the study - or something along those lines (since it is not only adding)

Copy link
Member Author

Choose a reason for hiding this comment

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

k

if user_level != 'admin':
admin_tags = to_add & system_tags_admin
if admin_tags:
message += ('Only admins can assing: '
Copy link
Contributor

Choose a reason for hiding this comment

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

assing -> assign

Copy link
Member Author

Choose a reason for hiding this comment

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

k

@wasade
Copy link
Contributor

wasade commented Mar 25, 2017

Seems reasonable

Copy link
Member Author

@antgonza antgonza left a comment

Choose a reason for hiding this comment

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

Thanks!

@@ -1152,6 +1137,75 @@ def unshare(self, user):
qdb.sql_connection.TRN.add(sql, [self._id, user.id])
qdb.sql_connection.TRN.execute()

def add_tags(self, user, tags):
Copy link
Member Author

Choose a reason for hiding this comment

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

k

Parameters
----------
user: User object
The user to unshare the study with
Copy link
Member Author

Choose a reason for hiding this comment

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

k

user: User object
The user to unshare the study with
tags : list of str
The tags to add to the study
Copy link
Member Author

Choose a reason for hiding this comment

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

k

if user_level != 'admin':
admin_tags = to_add & system_tags_admin
if admin_tags:
message += ('Only admins can assing: '
Copy link
Member Author

Choose a reason for hiding this comment

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

k

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 91.603% when pulling 56d8b0a on antgonza:adding-tags into 2795046 on biocore:master.

@antgonza antgonza mentioned this pull request Mar 27, 2017
2 tasks
Copy link
Contributor

@josenavas josenavas left a comment

Choose a reason for hiding this comment

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

Just one typo. I think the tag-it license should also be added in qiita_pet/static/vendor/licences. Otherwise looks good!

@@ -1152,6 +1137,75 @@ def unshare(self, user):
qdb.sql_connection.TRN.add(sql, [self._id, user.id])
qdb.sql_connection.TRN.execute()

def updata_tags(self, user, tags):
Copy link
Contributor

Choose a reason for hiding this comment

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

updata -> update

self.assertEqual(response.code, 200)
self.assertEqual(response.body, exp)

def test_patch(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

same question as in #2092, should there be negative tests?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think is required as all possible options are tested here: https://github.com/biocore/qiita/pull/2088/files#diff-a0b367a8e2e2e1d72fee16533d797415R541, what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is logically distinct. The goal of a negative test here, and with test_get, is to verify the status codes and response messages are rational for malformed or inappropriate REST requests

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 91.703% when pulling fd0b879 on antgonza:adding-tags into 2795046 on biocore:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 91.715% when pulling d6f5223 on antgonza:adding-tags into 2795046 on biocore:master.

@wasade wasade merged commit d3555ac into qiita-spots:master Mar 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants