learningequality / kolibri

Kolibri Learning Platform: the offline app for universal education
https://learningequality.org/kolibri/
MIT License
799 stars 655 forks source link

validation in 'importusersfromcsv' dry-run version should give different validation error when importing UUIDs for different facility #7192

Open jonboiser opened 4 years ago

jonboiser commented 4 years ago

Observed behavior

I exported a CSV from one facility and tried to import it into a new facility (after applying fixes in #7166 ), but would get these validation errors.

CleanShot 2020-06-25 at 17 21 10@2x

My guess is that this might be because I used the CSV with the UUID column filled in. And all the FacilityUsers with these IDs belong to a different facility, so the code is assuming I want to do an edit operation instead of a create operation.

Database ID (UUID) Username (USERNAME) Password (PASSWORD) Full name (FULL_NAME)
2ec1d6a0c73ab7fc6289f83eed37534f Pardslia * Yohannes H Tesfay
2d669107f2c4c99eb08fdbdbc9b80780 Wholed * Hayden Byrne E
1037a21a9d59e596dc656365e91b1aba Ventlestaked * Isabela R Sousa
2434a3c5940f5d717b22b43c8723ca16 Vate1974 * C Tá
e801a0a193599fff7fa03e63d5745849 Shichis * Ikenna Chialuka C
0a2dfca19bb32773bb185a42cace2d16 Frie1998 * Sokołowska
36d40c50b2949ff75249d70a9deb429f Antemblowind * Jikutokuruka
6dc6a3f5d6aea2cb4a28c25ab5314c2e Pecience * Văn Q
da4429e55ba6ad73101927df46a7a29c Ationvin * Esam H Saliba
9bfe56a1e0ceb2d741103e8bc0758a8b Inguld83 * Eyjólfsdóttir Elsa

Expected behavior

The workflow presents a more specific validation error in this situation. I was thrown off by the message focusing on the "Password" field, when it actually was the "UUID" field causing problems.

User-facing consequences

Errors and logs

Steps to reproduce

Context

v0.14.0-beta6

jredrejo commented 4 years ago

@jonboiser Honestly, I don't know how to manage this because I don't see the problem.

When importing, if you are going to create a new user you need to provide a password. Here you are not providing the password, so it's an error on that field.

The only situation where you can skip the password is when you're updating users, and this is not the case. The error is clearly that you are not providing the password for a non-existing user.

what is your proposal?

jonboiser commented 4 years ago

So I got that first user CSV by exporting it from Facility A, and I think that's why all the PASSWORD cells have an anonymized * password, but that's still a valid password for creating a new user.

The problem I had here is that when I try to import that CSV into a different facility (Facility B), I forgot to remove the UUID column, which is why I think all of them are getting "skipped". I believe I should have emptied the cells in the UUID column before importing to Facility B.

So the issue here is a UX one, because the validation error is making me think there is a problem with the PASSWORD column, when I really think it's a problem with the UUID column.

jredrejo commented 4 years ago

:man_shrugging: I still think the problem is in the PASSWORD column as it's the data the user is missing in the csv. In any case we can agree the problem, in this particular case, is with both columns UUID & PASSWORD.

I think the validation is correct, the doubt is deciding on the wording of the error message. If you have a better wording and it can be changed for 0.14 (are strings totally frozen now?) I will be happy to change it. In any case we should try to find some quorum for the changes as these strings have already been checked by several people.