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

Existing plant lines upload #781

Merged
merged 5 commits into from
Feb 28, 2018
Merged

Existing plant lines upload #781

merged 5 commits into from
Feb 28, 2018

Conversation

kammerer
Copy link
Member

@kammerer kammerer commented Feb 22, 2018

Partial fix for #756

Allows to include existing PLs in uploaded file provided submitted data matches existing records. Does not yet allow to update existing data as suggested by Wiktor in a comment to #756.

@kammerer kammerer force-pushed the existing-plant-lines-upload branch from 7577088 to 5d1efe3 Compare February 22, 2018 17:58
@kammerer kammerer requested a review from Nuanda February 22, 2018 17:58
@Nuanda
Copy link

Nuanda commented Feb 24, 2018

I'm not sure if this is a regression but when I delete a PL from the "Plant line list" field, it stays there hidden (reappears when reloading the page). This is especially not intuitive when I upload a CSV and the yellow box tell me to clear this "Plant line list" field before reuploading (to have the parser re-read and re-add a given PL). This does not work unless I "save" the form, after removing the PL from the list. Could that be helped?

Copy link

@Nuanda Nuanda left a comment

Choose a reason for hiding this comment

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

Specs pass on my setup. Three remarks for your consideration before merging.

@$('.fileinput-button').removeClass('disabled')

$errors = $(".errors").text("").removeClass('hidden').append("<ul></ul>")
$errors.find("ul").append($("<li></li>").text("Unexpected server response: #{data.jqXHR.status} #{data.jqXHR.statusText}"))
Copy link

Choose a reason for hiding this comment

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

This works - I was able to see a 500 error when I uploaded a wrong file. What is wrong, however, that I got that 500 in the first place. I tried to upload a completely wrong file and I caught BIP trying to select on nil:

NoMethodError - private method `select' called for nil:NilClass:
  app/decorators/submission_plant_lines_upload_decorator.rb:22:in `uploaded_new_plant_lines'
  app/decorators/submission_plant_lines_upload_decorator.rb:4:in `as_json'

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you remember what kind of file you uploaded? I only managed to get this 500 response using a badly encoded file, otherwise I get regular error message such as "File content type is invalid".

Copy link

Choose a reason for hiding this comment

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

I did it with two files, one was even a CSV, only a completely different one (than expected). I also remember I selected nothing on the 2nd step. The 3rd step was also empty when I uploaded the file.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, fixed.

@@ -62,6 +62,10 @@
columns</i>. Also, your organisation should ensure the unique naming of accession - so you never
duplicate the <b>Plant accession</b> value in the context of the same <b>Originating organisation</b>.

%p
When including plant lines already defined in BIP it is best to skip all optional information. Otherwise, any
conflicts with existing data will prevent plant lines from being included in submitted population.
Copy link

Choose a reason for hiding this comment

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

However, the parser prevents no-species rows. So I had to supply correct Species value for an existing PL, to have it accepted. Either change the description here or, preferred, make rows with existing PLs acceptable without Species value.

Copy link
Member Author

Choose a reason for hiding this comment

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

I made Species optional too.

@kammerer kammerer force-pushed the existing-plant-lines-upload branch from 5d1efe3 to e2ced0f Compare February 27, 2018 12:47
@kammerer kammerer force-pushed the existing-plant-lines-upload branch from 84cd1ff to 68dc259 Compare February 27, 2018 19:06
@kammerer kammerer force-pushed the existing-plant-lines-upload branch from c060d92 to f014dbe Compare February 27, 2018 19:31
@kammerer
Copy link
Member Author

I fixed this non-intuitive behaviour but I only managed to do that by allowing to freely override current content by file upload. This is not perfect but I could not think of any better way since the client-side and server-side state is not fully in sync here.

@Nuanda
Copy link

Nuanda commented Feb 28, 2018

Thanks for these fixes. The PR is already approved.

@kammerer kammerer merged commit a9990f5 into master Feb 28, 2018
@kammerer kammerer deleted the existing-plant-lines-upload branch February 28, 2018 16:18
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.

2 participants