search

cognoma / frontend

Frontend for Project Cognoma
http://cognoma.org/
Other
4 stars 22 forks source link

Revise Query Check #104

Closed rdvelazquez closed 7 years ago

rdvelazquez commented 7 years ago

This is a work in progress PR to addresses #102. I'm posting this now to:

  1. Let everyone know that I'm working on this and avoid duplicating work.
  2. Get some advice or guidance.

Once complete this PR will add an additional check so that users can’t submit queries if the query doesn’t have at least 20 positive and 20 negative samples. See #102 for the reasoning. I also revised the checks so that the classifier is validated before asking for an email address (hence the rearranging of things and the addition of the new function validateClassifier.

This is my first time working with the frontend so I’m not completely familiar with how things are set up. I left dummy variables in line 41 of QueryBuilder.js for the number of positive samples and number of negative samples in the query. I’m not sure the best way to get the number of positives and number of negatives… the numbers shown on cognoma.org under DISEASE TYPE SET are set by .setTotalFor in the following two files:

@dcgoss and/or @bdolly... I'll look into this a little more and update it if I find a working solution but any advice or guidance would be much appreciated.

rdvelazquez commented 7 years ago

This is now working on my local branch and is ready for a review. @bdolly and I met last night and he was able to show me how to get it working. Thanks!

@dhimmel do you want to get this merged before tomorrow night?

dhimmel commented 7 years ago

@dhimmel do you want to get this merged before tomorrow night?

That would be great! Thanks @rdvelazquez for dabbling in the frontend.

I'm not a great reviewer for this. @bdolly did you look at this and is it good to go?

rdvelazquez commented 7 years ago

@bdolly this is ready for your review/merge.

dhimmel commented 7 years ago

In case @bdolly is busy, I'm tagging @dongbohu (a full stack developer that's part of the Greene Lab and CCDL). Would love the get this reviewed and merged.

rdvelazquez commented 7 years ago

Thank you for your review @dongbohu! Very helpful comments!

Obviously there are many coding style issues in the whole code base (not only this PR). I pointed out a few but eventually gave up. I strongly recommend you guys set up some tools such as codeclimate to make the code more readable.

I agree that the coding style (as well as structural layout) does not lend itself to readability. I think something like code climate could be a good addition moving forward.

bdolly commented 7 years ago

Obviously there are many coding style issues in the whole code base (not only this PR). I pointed out a few but eventually gave up. I strongly recommend you guys set up some tools such as codeclimate to make the code more readable.

Totally agree and I think something like codeclimate is a great idea. However, seeing as how the codebase that exists was intended as MVP only and to be refactored into a more updated js framework (React, Angular, Vue) not sure that tooling would be worth the effort and clean-up at this time.

bdolly commented 7 years ago

@rdvelazquez @dongbohu All looks good to me, my only suggestion would be to make the notification bar stay open a bit longer so the user has more time to read it.

If both of you give this the thumbs up, I'll be happy to merge.

bdolly commented 7 years ago

@dhimmel I know the CI is going to bomb on this so it will likely need pushed to prod manually until the testing suite and CI is ironed out and fully implemented

dhimmel commented 7 years ago

until the testing suite and CI is ironed out and fully implemented

Automatic deployment would be really convenient. If I understand correctly, this repo will automatically deploy to AWS upon a successful build?

What is failing with the tests? Perhaps we should just skip the tests until they're fully implemented? The test library probably has some option to allow failure of a test.