Fix: CSV import removes existing userGroups memberships

[nshandra]

📌 References

• Issue: Closes [User feedback] Cannot update users from CSV

📝 Implementation

Round 2

After the merging of #295, a number bugs were discovered with the new table, aside of the one mentioned here.

• The "Overwrite existing users" switch didn't trigger a new validation of the form, leading to the "Import" button to be unresponsive.
• When a user was imported without search or dataView OUs the import failed with "POST 409" due to an incorrect transformation of emptyOrgUnit (entity) into an empty OU for data. This issue also happened when the instance OU doesn't have a code (its not mandatory).
• A new user could be imported with an empty password field.
• When a user was imported with empty fields for the columns of type role, group or OU it caused an uncaught exception.

To Do:

• Validate new users for repeated names (now it only checks for overwrite names). For now, if a username is duplicated the import will fail with "Usernames must be unique".

  I didn't manage to implement this via the form validations because the error appeared in the fields but the formState errors didn't updated until the next form interaction, i.e.: when writing a second duplicated username the field was set to "red" but the top corner error message appeared when writing a char in some field (username or other).
  @eperedo if you have time try to give it a go.

Round 1

A bug was reported that a CSV import will remove the imported users group membership. A subsequent import will restore the membership.

This bug is not dependent on the DHIS2 version and was replicated in DHIS 2.37 and 2.38.

The cause of the issue was that the users object had the following structure for userGroups :

{
  id: Id,
  displayName: string
}

When comparing with existingUsersToUpdate, its userGroups has only the id property.
This means that the comparison via _.differenceWith(existingUser?.userGroups, user.userGroups, _.isEqual) treats the same membership as a difference because of the displayName prop.

Now, the getUsersToSave function will return the user object with the unnecessary properties removed.

Also, a check was added for the metadata POST status to avoid running updateUserGroups if the POST fails.

📹 Screenshots/Screen capture

The capture starts after the CSV is loaded because the orgUnit call takes its time, the same CSV export of the user is used in both cases.

🔥 Testing

Export some users (optionally add a new one in CSV and/or import table), import them back, check that the userGroups are the same for the existing users after the import.

[eperedo]

It works (Tested on 2.38.6) thanks @nshandra

[nshandra]

Will comment here in case we use this PR to fix this:

A new bug was reported, this time the teiSearchOrganisationUnits field is emptied on import for existing users if its not provided in the CSV.

Looks like the source of the bug is in src/legacy/models/userHelpers.js:291

function getUserPayloadFromPlainAttributes(baseUser, userFields) {
    const clean = obj => _.omitBy(obj, value => value === undefined || value === null);

    const userRoot = {
        ...baseUser,
        ...clean(_(userFields).omit(userCredentialsFields).value()),
        id: baseUser.id || userFields.id,
    };

    return {
        ...userRoot,
        // userRoot.searchOrganisationsUnits is undefined on overwrite
        teiSearchOrganisationUnits: userRoot.searchOrganisationsUnits,
        userCredentials: {
            ...baseUser.userCredentials,
            ...clean(_(userFields).pick(userCredentialsFields).value()),
            id: (baseUser.userCredentials && baseUser.userCredentials.id) || generateUid(),
            userInfo: { id: userRoot.id },
        },
        ...clean(_(userFields).pick(user238MissingFields).value()),
    };
}

I guess the bug happened because the User.ts entity has searchOrganisationsUnits instead of teiSearchOrganisationUnits, leading to a prop name mistake.

@eperedo Do you know why it has a different name in the entity? I guess to separate entity from data.

[eperedo]

I guess to separate entity from data.

Yes, a clean arc. concept.

[eperedo]

Code looks good, thanks @nshandra. I've made some changes to include the validation for new users and a fix for the "ADD USER" button which was clearing the values entered in other rows.

[eperedo]

Nothing wrong here, but for this case we prefer to use a ternary operator.

return orgUnit.id === "" ? undefined : { ...orgUnit, path: joinPaths(orgUnit) };

[nshandra]

Thanks for the in-form duplicate usernames validation and the bug fix.