Template Generation improvements

aaclan-ebi commented 4 years ago

User story: As a contributor I want to generate a metadata template as quickly and easily as possible, so that I start preparing this for data submission.

lauraclarke commented 4 years ago

@ebi-ait/hca-wranglers where is the documentation about how to fill out the spreadsheet, it would be good to make sure some sort of linking happens to the spreadsheet template generator

mshadbolt commented 4 years ago

status update

tags as people that might be interested: @rdgoite @gabsie @clairerye @ami-day

Questionnaire in the ui

Questions have changed a bit in naming and formatting based on latest feedback from @gabsie (image below) and currently waiting to be reviewed/merged in this PR: https://github.com/ebi-ait/ingest-ui/pull/54

I have been running them locally and it seems to work but haven't tried deploying some of these changes yet.

Outstanding issues are:

checkboxes and radio buttons aren't a list (one per line) because we don't have ui input components that display in this way
I have made the changes within the html/ts/json files but don't understand enough about the unit tests to be able to update the spec.ts files
I am not sure how to adjust the 'Generate template' button to change it to 'download template' and make it display on a single line rather than breaking over two lines which makes the button look huge
The preference is to make all questions empty when someone comes to the form but I haven't been able to figure out how to make the radio buttons start blank
The cancel button that took you back to projects seems to have disappeared, not sure if we need to be able to navigate from this page back to my projects some how
the simplistic logic still means we don't get ideal results sometimes. e.g.
- If someone chooses 'Sequencing' and then also chooses 'Organoid/Cell line', the 'Organoid/cell line' answer needs to provide information about where to link the cell line/organoid entities, without knowing whether it is an imaging or sequencing experiment. Therefore, in the answer key you have to provide links to both sequencing and imaging entities, but there is no distinction between how you add links or how you add entities, so providing the a link because the organoid might be linked to an imaged_specimen or a cell_suspension. I was thinking this could be implemented as some way of specifying:

if the entity is already in the sheet: 
         add this link
else: 
         ignore this link

Another example is that if someone selects Human, I have included medical_history, height, height_unit, but really these modules are relevant only to experiments with Human AND No pre-natal samples, as in, an embryo doesn't have a medical history or a height. I had instead tried adding these only if the user selects either No, or Yes some, to the prenatal question, but that means that experiments with Adult Mice, will also get these fields, which are also not wanted in this circumstance. So if we don't want to change the way the logic works we will need to make a call on which kind of experiment we don't mind adding extra fields to.

I have a ticket for a general review of the logic but I have tried to go through the logic so many times in my brain I don't think I can offer much more and it would be great if someone else can review it to see if it makes sense to them

Actual spreadsheet generation

The PR that adjusts the row heights is here:https://github.com/ebi-ait/ingest-client/pull/6, it has been initially reviewed by Alegria but not sure if it is ready enough to merge and deploy through to the broke and into dev/staging.
The fix for the user-friendly names and too many required fields is currently on dev but not yet on staging I think.

I am not aware of any outstanding issues with generating the spreadsheet itself.

Testing

None of the changes above have been tested by anyone other than me so I would appreciate help with that. I had written tests into the test plan (https://docs.google.com/spreadsheets/d/1j_Q6p6h4k6xH_hF6nwtdb0k05W1mnq-ZKqLtMG_8LfA/edit#gid=0) but due to the changes in the form, many of these tests will need to be adjust as there is now different expected behaviour. But this isn't necessarily the only way to test things.

We can also/instead go through the process that was suggested by @gabsie where all the wranglers can think of a particular project that they want to generate a project for and see whether it does what they expect it to do.

Gabs' last framework image I was basing the edits to questions on above. Gabs framework

ami-day commented 4 years ago

It looks like you've done a lot of great work on this @mshadbolt! I'm going to be really honest and say that I am personally quite disappointed with this latest framework image (i'm sorry @gabsie). I loved how this page looked and worked before, and think the pre-filled options and ontology annotations were brilliant and would be very helpful for potential users and encourage them to use our platform. This new version seems a downgrade from what we had already implemented in my opinion. I'm not sure what others think about this, though. I will spend today looking through and checking how the version in both production and staging are looking/working.

ami-day commented 4 years ago

Just assigned us all on devs and whoever has contributed to this work so far so we can all keep up to date

aaclan-ebi commented 4 years ago

Maybe, we can support both. In the UI, a user can start from scratch (no fields filled), then there can be themes/pre-filled form values which will initialise the form based on chosen theme and user can further customise the answers.

aaclan-ebi commented 4 years ago

That will be very easy to implement. This is the change to remove the defaults.

https://github.com/ebi-ait/ingest-ui/pull/54/commits/072e698ff576137c9a6366b2abc601275eeafdb3#diff-2ac40356b394518bd8c827213af3c805L47-L53

we can let user choose their preference, to start from scratch or to start with some default values. Need to think about how to call those themes/templates (templates on templates)

ami-day commented 4 years ago

Maybe, we can support both. In the UI, a user can start from scratch (no fields filled), then there can be themes/pre-filled form values which will initialise the form based on chosen theme and user can further customise the answers.

Yes, I really like this idea. I understand that having the pre-filled options appear right from the start can be confusing if you haven't yet selected anything. But it's still really nice to be able to quickly see and select/click-on pre-selected options with ontology annotations, like how it was before.

ami-day commented 4 years ago

@aaclan-ebi how would the above look in the UI? is there a way I can compare it to what is already in production and staging e.g. in dev, without removing any changes Marion has already made?

lauraclarke commented 4 years ago

This feels like it might be a future improvement rather than something which could delay the release of the current system @clairerye is there a good play for @ami-day to record this idea?

aaclan-ebi commented 4 years ago

@ami-day it's not implemented yet, i just put the github commit link which can give us an idea how to implement that suggestion.

aaclan-ebi commented 4 years ago

yeah, we can create a ticket for that. Btw, this ticket is an epic created to group several tickets for the user story to improve template generation. This epic has been linked to a milestone for that sprint (and it seems that milestone was removed) Several tickets are already created linked to this epic.

ami-day commented 4 years ago

I really don't want to delay the release. I'm mainly referring to what had already been implemented and it sounds like the above improvement would be a small change. Will have a look at the current staging/prod. pages and then also look at the above proposed code change in detail. Obviously @gabsie , @clairerye and @lauraclarke would need to see how these changes look

ami-day commented 4 years ago

yeah, we can create a ticket for that. Btw, this ticket is an epic created to group several tickets for the user story to improve template generation. This epic has been linked to a milestone for that sprint (and it seems that milestone was removed) Several tickets are already created linked to this epic.

Oh, ok. I wasn't aware of this

aaclan-ebi commented 4 years ago

Yes that should be easy to implement, but one thing not clear is when is the release and when do we want all development done. We may want to finish any work before next sprint starts which is by 7 Oct.

ami-day commented 4 years ago

Yes, I'm pretty sure we agreed on by 7 Oct with Claire? Will update later once I have reviewed and checked the current working versions in staging and prod.

clairerye commented 4 years ago

Yes, you are correct @ami-day, we want this 'done for now' in time for starting the new sprint on the 7th. So I think that puts almost all changes out of scope. I think we want to be checking that it does what we expect and that is is in production ready for external users. @aaclan-ebi reading Marion's comments, are some of the changes not yet deployed or am I missing things?

ami-day commented 4 years ago

OK sounds good. I am working on this "I think we want to be checking that it does what we expect and that is is in production ready for external users.". It is already looking really good in staging in terms of the UI side: https://staging.contribute.data.humancellatlas.org/home.

Though, I am just going to feedback a few things I picked up which might be worth considering changing asap if it is not too much work, I will write them here:

ami-day commented 4 years ago

The main problem I see as critical before we can deploy to production: In the current Staging version, once you have clicked “Generate spreadsheet”, a “Success” message is shown and the spreadsheet is downloaded. However, below the success message, the “Generate your metadata spreadsheet” page is shown again, as though the user has not already completed this questionnaire. Is there a way to remove this duplicated page and only display the message? (pasting screenshot below). To provide more information/context for the user, we could add this to the existing message (“You can now start to add your sample and experimental metadata information to this spreadsheet. Here are some links to help you fill out this spreadsheet. Please do not hesitate to get in touch with a wrangler using the wrangler team email: wrangler-team@data.humancellatlas.org for guidance and we would be happy to support you”). Helper text and useful links about data wrangling were already present in a previous version of the UI template generator page, so this should not be a huge task to recover back. Though, I am not sure who would be best to do this. I am happy to work on these but it might be useful to borrow a developer who can do this more quickly.

ami-day commented 4 years ago

![Uploading Screenshot 2020-10-02 at 15.45.07.png…]()

ami-day commented 4 years ago

I am now going to start checking the actual generated spreadsheet is ok in detail, and will continue during early next week. We have already done this before, identified and resolved issues, so I am hoping it will all look ok. Will update with any critical issues I find, if any.

ami-day commented 4 years ago

Pasting screenshots of how it looks in staging currently below for your information

ami-day commented 4 years ago

Project registration page

![Uploading Screenshot 2020-10-02 at 15.39.42.png…]()

ami-day commented 4 years ago

Template spreadsheet generator page:

The pre-selected options are actually shown by default here (as discussed with @Alegria above) however I think that's fine for now given we will be doing user testing?

ami-day commented 4 years ago

I strongly advocate for keeping the UI side of the project registration page and metadata template generation page how they are illustrated here, already implemented in staging.

aaclan-ebi commented 3 years ago

@aaclan-ebi reading Marion's comments, are some of the changes not yet deployed or am I missing things?

I think there are 2 unmerged PR's atm (ingest-client and ingest-ui). I have some comments for improvement. But those are not blockers. I guess it's up to the person who's taking over Marion's work on those and merge those PR's could be @rdgoite @MightyAx @ami-day ?

ami-day commented 3 years ago

Update: outcome of our meeting

We will add this text below the template questionnaire: "This is a beta service, and we are constantly improving it. If you need any guidance or would like to offer any feedback, please contact us at wrangler-team@data.humancellatlas.org and we will do our best to support you."
The questionnaire duplication was intentional so that multiple metadata spreadsheets can be created. We will see if this is confusing for users during user testing and adjust the logic if necessary.
We will keep the pre-selected options in the project registration page and deploy the 'tick box' framework for the template questionnaire, for now, which may later be adjusted on the basis of user testing feedback.

ESapenaVentura commented 3 years ago

I have reviewed the template and the service a little bit, and I have some feedback (Don't know if this has been already addressed):

SPREADSHEET

Tabs are ordered by biomaterials --> files --> protocols. This is less than ideal, as a user is more likely to fill out the spreadsheet correctly by following their experiment structure (e.g. Donor --> collection protocol --> specimen etc)
Description for ontology modules is picked up from the description of the module (e.g. from the "text" property) which is really generic (e.g. for the collection protocol method, it states "A term that may be associated with a process-related ontology term.") I don't know if this was addressed, but since we only want to expose the .text property, ideally the text property would pick up the description, guidelines etc from the property in the type entity property instead of in the ontology module.
Linked entities are embedded at the end instead of being embedded at the beginning.

Service

This is not an issue more than something that annoyed me a little bit, but when you select an option (e.g. in the imaging or sequencing question) the box shows red for a split second and changes back immediately.

I haven't had time to test more, but as I understand from reading the ticket, this service should be released today?

ebi-ait / hca-ebi-dev-team