Princeton-LSI-ResearchComputing / tracebase

Mouse Metabolite Tracing Data Repository for the Rabinowitz Lab
MIT License
4 stars 1 forks source link

Excel template download #933

Closed hepcat72 closed 3 months ago

hepcat72 commented 3 months ago

Summary Change Description

Added a conversion of the dfs_dict to an excel file and added the ability of the results page to automatically download the file.

Note, I have not yet removed or hidden the errors/warnings section onn the resulting page. And I have a few other ideas for nice-to-have features:

Affected Issues/Pull Requests

Review Notes

See comments in-line.

Checklist

This pull request will be merged once the following requirements are met. The author and/or reviewers should uncheck any unmet requirements:

hepcat72 commented 3 months ago

Not sure what caused the messy log... Ugh.

hepcat72 commented 3 months ago

This PR provides the ability to download a sample sheet with sample names filled in. Neat solution to using the JS download of an embedded file. In testing, I generated a study.xlsx file from an accucor, which worked well. 👍🏻

Blocking issue: However, I then deleted a few lines and used it as an input file and I got an exception:

That's a great test to have done! However, I already encountered and fixed this in the work on the next PR which removes the errors when only adding data to a fully generated sample sheet. Had I realized it existed in the previous PR, I'd have fixed it in that branch. Sorry, I know you would prefer that to have been done in this PR. I just simply didn't discover it until the next chunk of work.

Also, just a comment from testing it. When trying it out, it seems potentially confusing to me since it asks for two files and it wasn't obvious I didn't need to the second one. Also, it wasn't obvious that I would be adding to the study file I uploaded. Perhaps making it separate steps would help with that, but we can get further feedback and mock-up some ideas before developing.

Yeah, the muted text probably won't get read. That's a reasonable insight. I think there's value in keeping it as a single step/form (but I'll make that case when it's later discussed). Although, it could be handled by either presentation (e.g. a red asterisk on the peak annotation field and not on the animal/sample field, or a clever checkbox next to the default output file like:

Output file:

And if they uncheck it, a file field is revealed below it. Simple, but less ambiguous.

hepcat72 commented 3 months ago

@lparsons Since the blocking issue is fixed in the next PR, I'll hold off on merging this until that PR is merged in here... That will make "more to review" here, but since there are no other changes here, all you'll need to do is check that the exception isn't thrown upon re-review...

hepcat72 commented 3 months ago

The exception checks were done in the child PR which was merged in here, so I am going to merge this.

hepcat72 commented 3 months ago

...After the CI checks pass...