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

fix #1837 #2061

Merged
merged 6 commits into from
Feb 3, 2017
Merged

fix #1837 #2061

merged 6 commits into from
Feb 3, 2017

Conversation

antgonza
Copy link
Member

Turns out that qiita_pet/templates/study_ajax/add_prep_artifact.html was left behind by mistake so removing it. Also, during testing I realized that you could send an unrelated study_id/prep_info_id and the system will not complain, added a raise and a test. However, this meant that a few of the tests not didn't make sense and to be honest it seems like they weren't testing anything important/new; let me know if I read this incorrectly. Finally, simplified the code a bit.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 91.458% when pulling 19a9dda on antgonza:fix-1837 into 9eb9dbb on biocore:master.

@antgonza
Copy link
Member Author

@tanaes, this is the functionality you were requesting, could you take a look?

@antgonza
Copy link
Member Author

While working on other stuff I realized that this functionality will be nice on all cases we have a run prefix so the files are auto-selected so decided to make it work that way:
my-first-video

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 91.526% when pulling 6f0dd71 on antgonza:fix-1837 into 9eb9dbb on biocore:master.

@josenavas
Copy link
Contributor

I am concerned of the approach taken here. We tried to do this at the beginning, but the problem is that you don't know which one is the barcode files and which one is the forward reads. For example, in the GIF that you have in the comments, it shows the R1 in barcodes and the R2 in forward reads, which I don't think is the correct assignment...

@antgonza
Copy link
Member Author

antgonza commented Feb 1, 2017

After offline discussion we agreed that this is the best we can come up with and that for all the real cases we know this will work. However, we should expand the Note to say that the user should verify the file distribution.

@antgonza
Copy link
Member Author

antgonza commented Feb 1, 2017

Message looks like this now:
screen shot 2017-02-01 at 2 25 09 pm

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 91.526% when pulling d9b41e8 on antgonza:fix-1837 into 9eb9dbb on biocore:master.

@josenavas
Copy link
Contributor

@tanaes can you review this PR when you have a second?

@josenavas josenavas requested a review from tanaes February 2, 2017 15:50

for k, v in viewitems(sfiles):
len_files = len(v)
if len_files != 1 and len_files != 2:
Copy link
Contributor

Choose a reason for hiding this comment

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

would it be possible to have a comment here about this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, added an explanation, improved the code and added a test.

<div class="blinking-message">
Please make sure that the correct files are in the correct column.<br/>
Note: the system will try to auto select the files based on run_prefix, if that doesn't work, either the type you selected doesn't support
it or the run_prefix is wrong
Copy link
Contributor

Choose a reason for hiding this comment

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

what is "it" here?

Copy link
Member Author

Choose a reason for hiding this comment

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

well, it's it ... fixing.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 8bf3d6e on antgonza:fix-1837 into ** on biocore:master**.

@josenavas josenavas merged commit 7807bac into qiita-spots:master Feb 3, 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