WIP: Fix DOI issues #2009

antgonza · 2016-12-05T23:04:16Z

Fixing DOI issues both in code & GUI and deleted add_publications cause is not used anywhere and doesn't apply with the new methods.

The new edit/create study looks like:

Missing stuff:

all tests pass?
add new db html/dbs

ackermag · 2016-12-05T23:30:18Z

Looks good to me

…

On Mon, Dec 5, 2016 at 3:04 PM, Antonio Gonzalez ***@***.***> wrote: Fixing DOI issues both in code & GUI and deleted add_publications cause is not used anywhere and doesn't apply with the new methods. The new edit/create study looks like: [image: screen shot 2016-12-05 at 3 48 50 pm] <https://cloud.githubusercontent.com/assets/2014559/20906242/4a2488d4-bb04-11e6-9bb3-ba72c9c5366f.png> Missing stuff: - all tests pass? - add new db html/dbs ------------------------------ You can view, comment on, or merge this pull request online at: #2009 Commit Summary - fix #1993, fix #1248 File Changes - *M* qiita_db/study.py <https://github.com/biocore/qiita/pull/2009/files#diff-0> (66) - *A* qiita_db/support_files/patches/44.sql <https://github.com/biocore/qiita/pull/2009/files#diff-1> (19) - *A* qiita_db/support_files/patches/python_patches/44.py <https://github.com/biocore/qiita/pull/2009/files#diff-2> (34) - *M* qiita_db/test/test_study.py <https://github.com/biocore/qiita/pull/2009/files#diff-3> (30) - *M* qiita_db/test/test_util.py <https://github.com/biocore/qiita/pull/2009/files#diff-4> (2) - *M* qiita_db/util.py <https://github.com/biocore/qiita/pull/2009/files#diff-5> (25) - *M* qiita_pet/handlers/api_proxy/studies.py <https://github.com/biocore/qiita/pull/2009/files#diff-6> (8) - *M* qiita_pet/handlers/api_proxy/tests/test_studies.py <https://github.com/biocore/qiita/pull/2009/files#diff-7> (27) - *M* qiita_pet/handlers/study_handlers/base.py <https://github.com/biocore/qiita/pull/2009/files#diff-8> (11) - *M* qiita_pet/handlers/study_handlers/edit_handlers.py <https://github.com/biocore/qiita/pull/2009/files#diff-9> (29) - *M* qiita_pet/handlers/study_handlers/listing_handlers.py <https://github.com/biocore/qiita/pull/2009/files#diff-10> (12) - *M* qiita_pet/handlers/study_handlers/tests/test_listing_handlers.py <https://github.com/biocore/qiita/pull/2009/files#diff-11> (21) - *M* qiita_pet/templates/edit_study.html <https://github.com/biocore/qiita/pull/2009/files#diff-12> (4) - *M* qiita_pet/templates/list_studies.html <https://github.com/biocore/qiita/pull/2009/files#diff-13> (8) Patch Links: - https://github.com/biocore/qiita/pull/2009.patch - https://github.com/biocore/qiita/pull/2009.diff — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#2009>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB69gOggDQn2fEJxLKSr_48mAgcLfQaOks5rFJhwgaJpZM4LE0pf> .

-- Gail Ackermann Knight Lab UCSD glackermann@ucsd.edu <ackermag@ucsd.edu>

josenavas · 2016-12-05T23:47:01Z

Sorry for jumping in a bit earlier than needed, but your last commit catch my attention. Is there any specific reason that we need a newer version of psql? Note that I'm not opposing to it, but if that's is required we will need to do some updates across all our systems, so these kind of updates should be handled carefully...

antgonza · 2016-12-05T23:48:11Z

Updating cause 9.6 is what we have in the systems, and cause we need something newer to retrieve ARRAY.

josenavas · 2016-12-08T00:27:42Z

qiita_db/study.py

+                     FROM qiita.study
+                     LEFT JOIN (
+                            SELECT study_id,
+                              array_agg(ARRAY[publication::text, is_doi::text])


I think all the issues that you're having started due to this call. Can you check if this construction helps addressing your issues w/o the need of changing the minimum version of postgres?

array_agg((publication, is_doi))

Note the double parenthesis. What do you think?

Thanks for the suggestion. I tried that before and just retested but it returns a string, which I don't like but if people agree with this I can use literal_eval from ast.

antgonza · 2016-12-13T23:08:05Z

OK, this is ready for review. I will like to hear back before changing the htmls for the DB, in case we need to change them again. Thanks!

BTW I went back to postgres 9.3 as is what we have in our servers.

coveralls · 2016-12-13T23:13:01Z

Coverage decreased (-0.0009%) to 91.447% when pulling 42ba6b3 on antgonza:fix-doi into 490045f on biocore:master.

josenavas

Minor comments on the patches

josenavas · 2016-12-15T18:14:05Z

qiita_db/support_files/patches/44.sql

+
+
+-- dropping PRIMARY KEY ( study_id, publication_doi )
+ALTER TABLE qiita.study_publication DROP CONSTRAINT idx_study_publication_0;


Should a new primary key be added on publication?

Thought about it and thought that study and is_doi should be it but at the same time I feel like it's not necessary, thus why I didn't add it. Please let me know if you feel strong about it.

josenavas · 2016-12-15T18:18:14Z

qiita_db/support_files/patches/python_patches/44.py

+
+
+# selecting all doi/pubmedids
+with qdb.sql_connection.TRN:


Any specific reason why this is executed in 2 different transactions?

No, will merge all of them

josenavas · 2016-12-15T18:18:39Z

qiita_db/support_files/patches/python_patches/44.py

+               LEFT JOIN qiita.publication AS p ON (sp.publication = p.doi)
+               WHERE p.doi NOT IN (
+                 SELECT publication_doi FROM qiita.software_publication)"""
+    qdb.sql_connection.TRN.add(sql, [])


No need for the second parameter: qdb.sql_connection.TRN.add(sql)

josenavas · 2016-12-15T18:19:59Z

qiita_db/support_files/patches/python_patches/44.py

+# deleting all references to start from scratch
+with qdb.sql_connection.TRN:
+    sql = """DELETE FROM qiita.study_publication"""
+    qdb.sql_connection.TRN.add(sql, [])


Same as above regarding the second parameter

antgonza

Thanks for the reviews, one more question and I'll update the db schema

antgonza · 2016-12-15T22:27:48Z

qiita_db/support_files/patches/44.sql

+
+
+-- dropping PRIMARY KEY ( study_id, publication_doi )
+ALTER TABLE qiita.study_publication DROP CONSTRAINT idx_study_publication_0;


Thought about it and thought that study and is_doi should be it but at the same time I feel like it's not necessary, thus why I didn't add it. Please let me know if you feel strong about it.

antgonza · 2016-12-15T22:31:53Z

qiita_db/support_files/patches/python_patches/44.py

+
+
+# selecting all doi/pubmedids
+with qdb.sql_connection.TRN:


No, will merge all of them

antgonza · 2016-12-15T22:31:57Z

qiita_db/support_files/patches/python_patches/44.py

+               LEFT JOIN qiita.publication AS p ON (sp.publication = p.doi)
+               WHERE p.doi NOT IN (
+                 SELECT publication_doi FROM qiita.software_publication)"""
+    qdb.sql_connection.TRN.add(sql, [])


antgonza · 2016-12-15T22:32:00Z

qiita_db/support_files/patches/python_patches/44.py

+# deleting all references to start from scratch
+with qdb.sql_connection.TRN:
+    sql = """DELETE FROM qiita.study_publication"""
+    qdb.sql_connection.TRN.add(sql, [])


coveralls · 2016-12-15T23:30:29Z

Coverage decreased (-0.01%) to 91.457% when pulling 8eb160a on antgonza:fix-doi into c5c38a6 on biocore:master.

coveralls · 2016-12-16T00:05:24Z

Coverage decreased (-0.01%) to 91.457% when pulling 4151658 on antgonza:fix-doi into c5c38a6 on biocore:master.

fix qiita-spots#1993, fix qiita-spots#1248

5474fc6

ElDeveloper self-requested a review December 7, 2016 22:28

josenavas reviewed Dec 8, 2016

View reviewed changes

antgonza force-pushed the fix-doi branch from b95bc51 to 5474fc6 Compare December 13, 2016 20:11

antgonza added 3 commits December 13, 2016 13:12

Merge branch 'master' of https://github.com/biocore/qiita into fix-doi

70d7e7a

fixing code

b487f27

fixing errors

42ba6b3

mortonjt approved these changes Dec 15, 2016

View reviewed changes

josenavas reviewed Dec 15, 2016

View reviewed changes

antgonza commented Dec 15, 2016

View reviewed changes

antgonza added 3 commits December 15, 2016 15:33

addresssing @josenavas comments

fb92642

renaming files

4b26d42

Merge branch 'master' of https://github.com/biocore/qiita into fix-doi

8eb160a

fixing dbs/html

4151658

josenavas merged commit c0b5a6f into qiita-spots:master Dec 16, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Fix DOI issues #2009

WIP: Fix DOI issues #2009

antgonza commented Dec 5, 2016 •

edited

Loading

ackermag commented Dec 5, 2016 via email

josenavas commented Dec 5, 2016

antgonza commented Dec 5, 2016 •

edited

Loading

josenavas Dec 8, 2016

antgonza Dec 8, 2016

antgonza commented Dec 13, 2016

coveralls commented Dec 13, 2016

josenavas left a comment

josenavas Dec 15, 2016

antgonza Dec 15, 2016

josenavas Dec 15, 2016

antgonza Dec 15, 2016

josenavas Dec 15, 2016

antgonza Dec 15, 2016

josenavas Dec 15, 2016

antgonza Dec 15, 2016

antgonza left a comment

antgonza Dec 15, 2016

antgonza Dec 15, 2016

antgonza Dec 15, 2016

antgonza Dec 15, 2016

coveralls commented Dec 15, 2016

coveralls commented Dec 16, 2016



		-- dropping PRIMARY KEY ( study_id, publication_doi )
		ALTER TABLE qiita.study_publication DROP CONSTRAINT idx_study_publication_0;

WIP: Fix DOI issues #2009

WIP: Fix DOI issues #2009

Conversation

antgonza commented Dec 5, 2016 • edited Loading

ackermag commented Dec 5, 2016 via email

josenavas commented Dec 5, 2016

antgonza commented Dec 5, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antgonza commented Dec 13, 2016

coveralls commented Dec 13, 2016

josenavas 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

antgonza 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

coveralls commented Dec 15, 2016

coveralls commented Dec 16, 2016

antgonza commented Dec 5, 2016 •

edited

Loading

antgonza commented Dec 5, 2016 •

edited

Loading