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

Issue 855 #900

Merged
merged 49 commits into from
Mar 27, 2015
Merged

Issue 855 #900

merged 49 commits into from
Mar 27, 2015

Conversation

josenavas
Copy link
Contributor

:feelsgood::feelsgood::feelsgood::feelsgood:

Fixes #855

I've moved the status column from the study object to the processed data and now the status of the study is inferred by the status of its processed data.

I've all the tests passing (at least on my machine) but I don't think the functionality is currently consistent!

The issue is that the metadata pipeline will show all the processed data for a given study, even if a given processed data is not public... Since this PR is making a deep change in the db structure and has impacts in a lot of parts of the codebase; I think the most secure way of moving this forward is making a PR so people can download the PR and check different contexts, in case that I missed something.

Missing things (that I can think of - please add as needed):

  • Modify interface so the make public / revert to sandbox / etc buttons are in per processed data basis.
  • The meta-analysis pipeline should only show the processed data that is public or that the user is the owner (see The meta-analysis pipeline should only show the processed data... #1005)
  • If accessing a public study, the study description page should only show the public raw and (pre)processed data from the public processed data.
  • The studies awaiting approval page for admins should show which processed data is awaiting approval.

@wasade
Copy link
Contributor

wasade commented Mar 11, 2015

@josenavas, conflicts

JOIN qiita.processed_data_status pds
ON pds.processed_data_status_id=pd.processed_data_status_id
WHERE pds.processed_data_status=%s"""
studies = [x[0] for x in
Copy link
Contributor

Choose a reason for hiding this comment

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

can this be a set? See #961

@squirrelo squirrelo mentioned this pull request Mar 16, 2015
@ElDeveloper
Copy link
Member

I think this is looking good, and it works great!

The only other thing that I noticed (aside from the minor comments above), is that the icon in the "show prep template summary" button should probably removed, as the eye or the crossed-out eye imply information about the privacy status of the data objects.

screen shot 2015-03-25 at 3 31 56 pm

@ElDeveloper
Copy link
Member

Forgot to add, 👍.

@josenavas
Copy link
Contributor Author

Good catch @ElDeveloper I've also removed the eye icon from the show sample template summary so the interface is consistent. I tried to find another icon, but given that we already have the test and I cannot find another "good enough" icon, I just left the text. Thanks for the reviews!

@ElDeveloper
Copy link
Member

Thanks!! That seems reasonable (re: button). 👍

Yoshiki Vázquez-Baeza

On Mar 25, 2015, at 8:10 PM, josenavas notifications@github.com wrote:

Good catch @ElDeveloper I've also removed the eye icon from the show sample template summary so the interface is consistent. I tried to find another icon, but given that we already have the test and I cannot find another "good enough" icon, I just left the text. Thanks for the reviews!


Reply to this email directly or view it on GitHub.

@josenavas
Copy link
Contributor Author

That's a new one! I received a "Killed" message... I re-started the build...

@ElDeveloper
Copy link
Member

Yeah, I was getting that last week. I've started taking note of the
worker node identifiers and the output of redis-server --version
(maybe there's something funky there. BTW, I restarted the build
(again).

On (Mar-25-15|21:49), josenavas wrote:

That's a new one! I received a "Killed" message... I re-started the build...


Reply to this email directly or view it on GitHub:
#900 (comment)

@@ -14,4 +14,14 @@
</tr>
{% end %}
</table>
{% if btn_to_show == 'request_approval' %}
<a class="btn btn-default glyphicon glyphicon-eye-open" onClick="request_approval({{pd_id}});" style="word-spacing: -10px;"> Request Approval</a>
Copy link
Member

Choose a reason for hiding this comment

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

Can you be more specific, users will not know what approval means? Perhaps, "Request making this data public". Same to the other buttons.

@antgonza
Copy link
Member

IMOO it's confusing that awaiting for awaiting for private approval has an orange eye and when public a green one. The confusing part is that private has a gray eye. What about: waiting for approval: orange open eye + 'glyphicon glyphicon-lock', private yellow open eye + 'glyphicon glyphicon-lock', public green open eye + 'glyphicon glyphicon-globe'?

@antgonza
Copy link
Member

Two comments. After those 👍

@ElDeveloper
Copy link
Member

Build restarted ⏰

@antgonza
Copy link
Member

... and again.

@josenavas
Copy link
Contributor Author

This is getting annoying... we need to find a solution for that...

@josenavas
Copy link
Contributor Author

I've increased the timeout time to 10 sec instead of 5, to see if now the tests pass....

@josenavas
Copy link
Contributor Author

Now a redis connection problem 😢 re-starting...

@josenavas
Copy link
Contributor Author

Ok, I got the tests passing now. I'm going to re-start them once to see if they are passing again. If so, this should be ready to final review/merge

@coveralls
Copy link

Coverage Status

Coverage increased (+0.16%) to 79.09% when pulling 46b84ef on josenavas:issue-855 into 7a398ed on biocore:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.16%) to 79.09% when pulling 3bca14e on josenavas:issue-855 into 7a398ed on biocore:master.

ElDeveloper added a commit that referenced this pull request Mar 27, 2015
@ElDeveloper ElDeveloper merged commit 54f63e9 into qiita-spots:master Mar 27, 2015
@ElDeveloper
Copy link
Member

Thanks @josenavas!

@josenavas
Copy link
Contributor Author

💥

@coveralls
Copy link

Coverage Status

Coverage increased (+8.6%) to 87.539% when pulling 3bca14e on josenavas:issue-855 into 7a398ed on biocore:master.

@ElDeveloper
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

shared status shouldn't be by study but by processed data
7 participants