database changes to fix 969 #2071
Conversation
|
|
||
| Parameters | ||
| ---------- | ||
| tag_ids : list of int |
There was a problem hiding this comment.
Can we provide the list of tags rather than the list of tag ids? We have been removing the exposure of DB ids for these types of thins as much as we could, so I would like not to re-add some of them back
There was a problem hiding this comment.
Just FYI: We (@josenavas and me) decided via offline chat that it was fine to leave as is because that way we can link the ids in the templates and the text can be escaped in the templates, which makes our live easier.
| return qdb.sql_connection.TRN.execute_fetchindex() | ||
|
|
||
| @tags.setter | ||
| def tags(self, tag_ids): |
There was a problem hiding this comment.
This is not setting the tags correctly. If a tag has been removed from the list it doesn't get removed from the DB.
Would it be more useful to add a couple of functions add_tags and remove_tags?
| The study tags | ||
| """ | ||
| with qdb.sql_connection.TRN: | ||
| sql = """SELECT study_tag_id, study_tag |
There was a problem hiding this comment.
Just return the "study_tag" (not the id)
qiita_db/test/test_study.py
Outdated
|
|
||
| # assigning the tags to study | ||
| study = qdb.study.Study(1) | ||
| study.tags = [tig for tig, tag in obs] |
There was a problem hiding this comment.
can you add a:
self.asertEqual(study.tags, [])before this line
qiita_db/test/test_study.py
Outdated
| study = qdb.study.Study(1) | ||
| study.tags = [tig for tig, tag in obs] | ||
| # and checking that everything went fine | ||
| self.assertEqual(exp, study.tags) |
There was a problem hiding this comment.
Add another case in which a tag is removed from the list.
|
|
||
| Parameters | ||
| ---------- | ||
| tag_ids : list of int |
qiita_db/study.py
Outdated
| qdb.sql_connection.TRN.execute() | ||
|
|
||
| if tag_ids: | ||
| sql = """INSERT INTO qiita.per_study_tags |
There was a problem hiding this comment.
If something goes wrong when this query is executed, we will have lost the tags 😱. IIRC the API should allow you to only execute at the very end of this function, such that if something fails the whole transaction is rolled back.
There was a problem hiding this comment.
good point, deleting this execute
qiita_db/study.py
Outdated
| )""" | ||
| sql_args = [[tid, self._id, tid, self._id] for tid in tag_ids] | ||
| qdb.sql_connection.TRN.add(sql, sql_args, many=True) | ||
| return qdb.sql_connection.TRN.execute() |
There was a problem hiding this comment.
What's being returned here? ... should anything be returned at all?
There was a problem hiding this comment.
good point, removing the return
| CREATE TABLE qiita.study_tags ( | ||
| study_tag_id bigserial NOT NULL, | ||
| email varchar NOT NULL, | ||
| study_tag varchar NOT NULL, |
There was a problem hiding this comment.
Can/should we guarantee unique tags?
There was a problem hiding this comment.
yes, 1 line below: CONSTRAINT pk_study_tag UNIQUE ( study_tag_id ),
|
|
||
| Parameters | ||
| ---------- | ||
| tag_ids : list of int |
There was a problem hiding this comment.
Just FYI: We (@josenavas and me) decided via offline chat that it was fine to leave as is because that way we can link the ids in the templates and the text can be escaped in the templates, which makes our live easier.
qiita_db/study.py
Outdated
| qdb.sql_connection.TRN.execute() | ||
|
|
||
| if tag_ids: | ||
| sql = """INSERT INTO qiita.per_study_tags |
There was a problem hiding this comment.
good point, deleting this execute
qiita_db/study.py
Outdated
| )""" | ||
| sql_args = [[tid, self._id, tid, self._id] for tid in tag_ids] | ||
| qdb.sql_connection.TRN.add(sql, sql_args, many=True) | ||
| return qdb.sql_connection.TRN.execute() |
There was a problem hiding this comment.
good point, removing the return
| CREATE TABLE qiita.study_tags ( | ||
| study_tag_id bigserial NOT NULL, | ||
| email varchar NOT NULL, | ||
| study_tag varchar NOT NULL, |
There was a problem hiding this comment.
yes, 1 line below: CONSTRAINT pk_study_tag UNIQUE ( study_tag_id ),
These are the DB changes so we can add the tagging system to Qiita. Note that I decided to keep track of which user adds which tag so we can allow users to add tags and still catch any abuse.