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

Batched sample registration form with Paste capabilities #2658

Open
wants to merge 16 commits into
base: 2.x
Choose a base branch
from

Conversation

ramonski
Copy link
Contributor

Description of the issue/feature this PR addresses

This PR enhances the sample add form for better usability and data management

Current behavior before PR

  • All sample columns are displayed
  • Only possible to copy the value of the first column over to the other columns
Old Sample Add Form

Desired behavior after PR is merged

  • Tabbed sample columns that allow individual navigation
  • Paste window to enter individual values for sample columns
New Sample Add Form

--
I confirm I have tested this PR thoroughly and coded it according to PEP8
and Plone's Python styleguide standards.

@ramonski
Copy link
Contributor Author

Something that might be confusing with the tabbed view is the "copy" button behavior.

It always uses the value of the first column (Sample 1) and copy it over to the other columns.

When you view now e.g. "Sample 3" and click this button, one might expect that the current displayed value is copied over to the other columns on the right, but that is not the case.

Hence, we might think of either hiding this >> button when switching to any other tab than "Sample 1" or adapt the functionality to copy always the value of the current displayed tab to the other columns.

What do you think?

@xispa
Copy link
Member

xispa commented Dec 11, 2024

Something that might be confusing with the tabbed view is the "copy" button behavior.

It always uses the value of the first column (Sample 1) and copy it over to the other columns.

When you view now e.g. "Sample 3" and click this button, one might expect that the current displayed value is copied over to the other columns on the right, but that is not the case.

Hence, we might think of either hiding this >> button when switching to any other tab than "Sample 1" or adapt the functionality to copy always the value of the current displayed tab to the other columns.

What do you think?

If it is not too difficult, I would adapt the functionality to always copy the value displayed in current tab. I think is the less confusing approach, yet the most powerful. Would hide the button when the current tab is not the first otherwise.

If you go ahead with adapting the current functionality, when the button is clicked and current tab is not the first, I would also display a confirmation pop-up:

The system will copy the value '<field_value>' from field '<field_name>' across the rest of samples.
Are you sure?
[Yes] [No]

@ramonski
Copy link
Contributor Author

Thanks for your feedback Jordi!
IMO a confirmation dialog is only necessary when the functionality is not intuitive and logical and might get annoying otherwise.
Therefore, I would like to implement the copy functionality for the current tab only to all columns on the right. I guess this should be possible because the tab switching and the copy functionality are implemented in JS.

@ramonski
Copy link
Contributor Author

Of course I would change the title attribute of the copy button for an updated description. But I guess the new button icon (>>) should hint enough what the functionality is doing, aka. copy value to the right.

@ramonski
Copy link
Contributor Author

Ok, implemented the "copy to columns on the right" feature and the PR is ready for review again

@xispa
Copy link
Member

xispa commented Dec 18, 2024

Man this is coool!. Haven't reviewed in detail yet, but I wanted to share a concern about the overlay for the introduction of individual values all-at-once:

Captura de 2024-12-18 16-14-46

This component is not specific of field type. User can manually type the text value for each sample. This works great for text-based field, but I think is prone to errors for fields from other types (e.g. select, reference, checkbox, etc.). For instance, if the field is a reference field and the text does not match any value, the system simply flushes the value of the field. And this might be a problem, specially taking into account that form values would usually remain "hidden" under their own tabs. Other examples in wihch this component might lead to confusion are select fields or custom-widget fields like patient's MRN or Date of birth.

Therefore I think would be better to display the icon for the introduction of individual values all-at-once only for text-based fields. However, I presume that some labs might want this functionality to be available even for fields not fully-compatible with the type (e.g. datetime), maybe because they do a copy&paste from a spreadsheet or similar that comes with the right format from the beginning. So an alternative would be to rely on a senaite registry setting to either render the icon or not for a given sample field.

What do you think?

@ramonski
Copy link
Contributor Author

Yes, this paste option might get tricky with these custom fields. Currently, this code handles it: https://github.com/senaite/senaite.core/pull/2658/files#diff-e3237269d12664a4fc5c68bf03f6cc8b5ef8cd8a82b36168beb3d48ed05792ffR983-R1018

paste_value: (field, value) =>
    # handle reference field
    controller = @get_widget_controller(field)
    if controller
      # NOTE: We use here the search API to find the entered text and set it
      # only when we find exactly one result
      promise = controller.search value
      promise.then (data) ->
        # only set the value if we get exactly 1 search result
        if data["count"] != 1
          return
        # select the result
        items = data["items"]
        controller.select(items)
        controller.clear_results()
    # set all other fields
    else if @is_text(field) or @is_textarea(field)
      field.value = value
    else if @is_radio(field) or @is_checkbox(field)
      try
        checked = Boolean(JSON.parse(value))
      catch
        checked = no
      field.checked = checked
    # date field
    else if @is_date_widget(field)
      # we need to fetch the date and time input fields
      date_input = field.querySelector("input[type='date']")
      time_input = field.querySelector("input[type='time']")
      try
        date = new Date(value)
        date_input.value = date.toISOString().split("T")[0]
        time_input.value = date.toTimeString().split(" ")[0]
      catch error
        console.warn error
        site.add_notification("Invalid date format", "Please use the format yyyy-mm-dd MM:HH")

This means that only exact matches for all reference fields are used, otherwise, the field is skipped.

Besides this, Checkbox, Text and Date fields can be (somehow) handled.

Maybe you can give it a try with the Patient fields and see what side-effects happen. If it get's too weird, we can maybe disable the button based on a registry setting...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants