WORK IN PROGRESS - Show genotype parsing status to users #472
Conversation
This reverts commit e656ac0.
Since we now display the parsing status to the user, it makes sence to do this in a predictable manner.
| @@ -0,0 +1,48 @@ | |||
| RSpec.describe Preparsing do | |||
There was a problem hiding this comment.
Block has too many lines. [37/25]
Missing magic comment # frozen_string_literal: true.
app/workers/preparsing.rb
Outdated
| @@ -134,5 +129,8 @@ def perform(genotype_id) | |||
|
|
|||
| Parsing.perform_async(genotype.id) | |||
| end | |||
| rescue | |||
There was a problem hiding this comment.
Avoid rescuing without specifying an error class.
spec/workers/parsing_spec.rb
Outdated
| @@ -1,18 +1,52 @@ | |||
| # frozen_string_literal: true | |||
| # TODO: Add test cases for different filetypes. | |||
There was a problem hiding this comment.
Add an empty line after magic comments.
| @@ -1,5 +1,5 @@ | |||
| # frozen_string_literal: true | |||
| RSpec.feature 'Delete a genotype', sidekiq: :inline do | |||
| RSpec.feature 'Delete a genotype', :js, sidekiq: :inline do | |||
There was a problem hiding this comment.
Add an empty line after magic comments.
| @@ -0,0 +1,5 @@ | |||
| class AddParseStatusToGenotypes < ActiveRecord::Migration | |||
There was a problem hiding this comment.
Missing magic comment # frozen_string_literal: true.
| @@ -0,0 +1,11 @@ | |||
| module UsersHelper | |||
There was a problem hiding this comment.
Missing magic comment # frozen_string_literal: true.
| @@ -0,0 +1,48 @@ | |||
| RSpec.describe Preparsing do | |||
There was a problem hiding this comment.
Block has too many lines. [37/25]
Missing magic comment # frozen_string_literal: true.
There was a problem hiding this comment.
I added the comment and changed the rubocop config to make it ignore block length in the spec directory.
app/workers/preparsing.rb
Outdated
| @@ -134,5 +129,8 @@ def perform(genotype_id) | |||
|
|
|||
| Parsing.perform_async(genotype.id) | |||
| end | |||
| rescue | |||
There was a problem hiding this comment.
Avoid rescuing without specifying an error class.
spec/workers/parsing_spec.rb
Outdated
| @@ -1,18 +1,52 @@ | |||
| # frozen_string_literal: true | |||
| # TODO: Add test cases for different filetypes. | |||
There was a problem hiding this comment.
Add an empty line after magic comments.
| @@ -1,5 +1,5 @@ | |||
| # frozen_string_literal: true | |||
| RSpec.feature 'Delete a genotype', sidekiq: :inline do | |||
| RSpec.feature 'Delete a genotype', :js, sidekiq: :inline do | |||
There was a problem hiding this comment.
Add an empty line after magic comments.
| @@ -0,0 +1,5 @@ | |||
| class AddParseStatusToGenotypes < ActiveRecord::Migration | |||
There was a problem hiding this comment.
Missing magic comment # frozen_string_literal: true.
| @@ -0,0 +1,11 @@ | |||
| module UsersHelper | |||
There was a problem hiding this comment.
Missing magic comment # frozen_string_literal: true.
| else | ||
| UserMailer.parsing_error(genotype.user_id).deliver_later | ||
| logger.info "file is not ok, sending email" | ||
| system("rm #{Rails.root}/public/data/#{genotype.fs_filename}") | ||
| Genotype.find_by_id(genotype.id).delete |
There was a problem hiding this comment.
I could use some feedback on this. If we delete the genotype on error, we cannot show the status to the user. What do we want to do as an alternative?
a) We could just not delete it.
b) We could delete it after a period of time and tell the user on the genotypes tab in their settings that we do.
c) Still delete it.
d) Any other ideas?
There was a problem hiding this comment.
What would the status display on error? Just nothing, or an 'error' thing? Ideally I'd like to delete it but we're not particularly low on space
There was a problem hiding this comment.
So far it says "error". I was thinking of adding more information, since sometimes we know what went wrong. We also have the emails we send in some cases but not others. We could make it one email and put the same state/error message in there.
There was a problem hiding this comment.
Regarding this.. I'm not 100% familiar with how the genotypes are set up in the database, but is it possible to:
Delete the genotype file
Delete most of the genotype data in the db, but leave behind the status field value and any other bare necessities required for displaying status to the user?
There was a problem hiding this comment.
Yep, that should be easily possible. The files themselves are attached to the Genotype model through paperclip. The code for our model is here.
With paperclip you can just set the genotype attribute of a Genotype object to nil and save the object. This should delete the file while keeping the model:
@genotype = Genotype.find(1)
@genotype.genotype = nil
@genotype.save
|
Just curious... what is currently holding this up? It'd be really nice to have this! I'm thinking about just merging it into my local copy... is it functionally in pretty good shape? |
|
What's holding this up is time constraints on my part and I think making decisions. See these comments. Feel free to continue working on this. It would be a great help! @philippbayer @gedankenstuecke Or what do you think? |
|
Oh, totally agree, all contributions are welcome! 👍 |
|
All contributions are good! To continue the old thread, how about for now we delete the genotyping, finish this, and then decide what to do with the errors? |
|
You mean deleting the files themselves or just the objects and re-parsing them? |
|
So far we completely delete the Genotype records, if parsing fails. This doesn't work well with showing their error status in the list of the user's uploaded genotypes. Keeping the files also isn't useful, I guess, so one way of dealing with this would be to keep the records, delete the file, and filter by success status in all other places. |
|
Ah, my bad. I'm too jet lagged for this. I was mentally still at the zip/gzip/etc. issue and how to work with the files we already have that are acting up. 😂 |
No description provided.