ChildMindInstitute / mindlogger-admin-OLD-Vue

Browser-based interface for administering the MindLogger platform
https://admin-prod.mindlogger.org
Other
4 stars 4 forks source link

The CSV template is not parsed if it was downloaded, edited in Excel and uploaded again #1643

Closed natalia-muzyka closed 2 years ago

natalia-muzyka commented 2 years ago

Preconditions The user is logged in on the admin panel

Steps to reproduce

  1. Open a site https://admin-staging.mindlogger.org/
  2. Open the General calendar of the applet
  3. Click the "Import schedule" button
  4. Download the template
  5. Edit template with the valid data in the Excel
  6. Save and upload a CSV to the Calendar
  7. Observe the result

Video: https://www.screencast.com/t/qc7g7gzbxbo

NOTES: The same behavior is for CSV schedule and Configurable Flanker (Blocks sequences).

The file is parsed successfully if CSV:

@WorldImpex If it's an expected behavior then suggest adding an additional info text to avoid confusion.

Environment: https://admin-staging.mindlogger.org/ Win 10 / Chrome 102 jeligi9407@zneep.com / 123456 Flanker v4 Applet password: Qwe123!!!

natalia-muzyka commented 2 years ago

image.png

Resolved: The info tooltip is added to the "Upload" button

Environment: https://admin-staging.mindlogger.org/ Win 10 / Chrome 102

natalia-muzyka commented 2 years ago

It is not possible to create the right CSV with only excel, you always need to use notepad in addition. All the column names and values are required to be in "". If let's say Notifications are specified for one event, then there should be an empty record "" for all the other events.

Our users are going to use Excel as the main tool for creating a CSV schedule or block sequences.

Suggestion: Creating a CSV should be easier and not require putting values in "". It should be possible to download a template, edit it with Excel, and upload it. As well as create a totally new CSV file. There could be used other separating characters than a comma. To prevent parsing issues depending on the separating character this character should be restricted to add in: activity name, secret user id, file name (flanker stimulus image, fixation image).

iradchenk0 commented 2 years ago

@binarybottle @WorldImpex

So, looks like it's not an issue with a code, it's all about the editing .csv file with different table editors I've tried to edit valid .csv file with Excel and LibreOffice Calc (with and without type formatting), and I've got very different results, and none of them is valid to csv parsing image.png image.png image.png

My questions and proposal:

  1. Is it necessary to use CSV format? Why?
  2. Natalia mentioned, that stakeholders mostly use Excel, so if CSV is not necessary, why don't we parse XLS tables? We want to fill the table in the "import schedule" dialog, so it's rationally to parse table extension
natalia-muzyka commented 2 years ago

@iradchenk0 Positive cases are passed for calendar. Only the validation pop-ups are missing after uploading xls, xlsx, ods with invalid values. Video: https://www.screencast.com/t/sSH8nRtCiosG

Test files: https://drive.google.com/drive/folders/1_upJ4gZdqdOJ7vyRUWQ1BKQrX76rM6e1?usp=sharing

Environment: https://admin-staging.mindlogger.org/ Win 10 / Chrome 106 new_user@ml.com / 123456 / Schedule check

natalia-muzyka commented 2 years ago

Noticed a few issues for XLS, XLSX, and ODS while retesting (differences with CSV):

1 - Safari doesn't parse uploaded ODS, XLS, XLSX (only CSV is parsed) at all - https://www.screencast.com/t/2RPCsIAw

2 - repeats empty frequency valid - general pop-up is shown instead of the specified image.png

3 - 01.01.2021 - no pop-up, no parsing - https://www.screencast.com/t/Or8hsxg8qc - expectation: show general error, don't parse any date format except mm/dd/yyyy

4 - date empty - no pop-up, no parsing - https://www.screencast.com/t/tVuEtjnNw5 - expectation: show general error, don't parse

5 - "undefined" in pop-up if activity name is empty - expectation: show general pop up, don't parse image.png

6 - incorrectly converted file doesn't trigger general error pop-up - https://www.screencast.com/t/42B3pxeVI

7 - in some pop-ups there is still "CSV" text part - suggest replacing it with "file" image.png

Test files: https://drive.google.com/drive/folders/18hqUv1DXNYvN4ElFwhbUqlqVuMxh6Q6l?usp=sharing

To-be suggestions (will be a new ticket created in JIRA, as it is not implemented at all): 8 - make a list of errors if there are more than 1 9 - don't let notification time be shorter than 4 symbols (now parses 111) should be 0000 =< time <= 2359 10 - 02/29/2022 , 11/31/2022 (not existing date) - show general error

Environment: https://admin-staging.mindlogger.org/ Win 10 / Chrome 106 macOS 10.15 / Chrome 106 / Safari 13.1.2 (15609.3.5.1.3) new_user@ml.com / 123456 / Schedule check

natalia-muzyka commented 2 years ago

@binarybottle @eleonova-scn please, take a look at my comment above and help us to define priorities. I think it would be better to fix before refactoring the issue #1, and if it's not a big effort and time-consuming task: 3, 4, 6. The others I think can wait until the refactoring will be completed.

binarybottle commented 2 years ago

I hate it when error messages don't pop up and things just fail. Any of the above that would be very quick to fix would be great to knock out before refactoring. Otherwise, if we simply add messaging that you have to edit in a Notepad-like program and not Excel, that will be fine as well for the coming months.

iradchenk0 commented 2 years ago

Fixed all commented points, but not sure if #1 is fixed, can't test it because of absence of macbook, so will wait for QA to test

Additionally fixed #9 and added new validation for invalid dates (#10)

8 Needs more time, better to postpone

yzenchanka commented 2 years ago

Issues 1-5, 7, 9, 10 verified as fixed

Environment: https://admin-staging.mindlogger.org/ Win 10 / Chrome 106 Issue #1 tested on macOS Monterey 12.0.1 / Safari 15.1 new_user@ml.com / 123456 / Schedule check

natalia-muzyka commented 2 years ago

Issues 1, 2, 4, 5, 7, 9, 10 verified as fixed on macOS 10.15 / Safari 13.1.2

@iradchenk0

3 date format 01.01.2021 - no pop-up, no parsing:

We have different behavior depending on the data type now. If it is Date then it is parsed, but 01.01.2021 becomes 12/31/2020 (expected 01/01/2021), or 01.11.2022 becomes 31.10.2022 (expected 11/01/2022). If it is General/Text then it is not parsed (general error is shown). If its All formats, then I'm getting an infinity loader and error in console (check video 2). Maybe it will be better to have only one option for date format to not increase the scope? MM/DD/YYYY Video 1: https://www.screencast.com/t/qgua0f2oTo Video 2: https://www.screencast.com/t/1eVNxBXS

Environment: https://admin-staging.mindlogger.org/ new_user@ml.com / 123456 / Schedule check

natalia-muzyka commented 2 years ago

6 is fixed

For #3

Verified and closed.

Environment: https://admin-staging.mindlogger.org/ Win 10 / Chrome 107 new_user@ml.com / 123456 / Schedule check