fix(current-user): allow custom fields in getUserGroups (DHIS2-10625)

[edoardo]

Improvement proposal for fixing DHIS2-10625.

This allows to override fields=:all to reduce the response size.
One example is the Interpretations component, where only the group ids
are actually needed.

See dhis2/d2-ui#685.

[HendrikThePendric]

Fixing things this way doesn't introduce breaking changes, so that's a good thing. However, this does obviously mean that everywhere in our code base we will have to start adding a fields arg to the call to getCurrentUser, because we should assume that getting all the users for a number of userGroups is going to cause a performance issue.

Perhaps we should include !users in the default fields filter?

(I'm just leaving a neutral review, because I'm not sure what would be best here)

[varl]

I don't understand why the support for optional params means we have to start to add the fields arg to the getCurrentUser?

It looks like everything will work the same as it does today if listOptions isn't passed along as an argument.

[varl]

If the intention is to allow overwriting the filter property, then the listOptions should be the last argument in the assign.

[edoardo]

True, I used the same approach already in place for getOrganisationUnits.
I don't think the filter should be overridden, these methods are called like d2.currentUser.getUserGroups() which implies the groups returned are the ones where the current user belongs.

[HendrikThePendric]

I think you make a good point, but we probably need a slightly more nuanced solution:

{
    paging: false,
    ...listOptions,
    filter: [
        ...listOptions.filter,
        `id:in:[${userGroupIds.join(',')}]`,
     ]
 }   

My point: the part where we filter the userGroups on the user's userGroups should not be overwritten.

Note: when I wrote the above, Edoardo's comment was not visible yet.

[edoardo]

Fixing things this way doesn't introduce breaking changes, so that's a good thing. However, this does obviously mean that everywhere in our code base we will have to start adding a fields arg to the call to getCurrentUser, because we should assume that getting all the users for a number of userGroups is going to cause a performance issue.

My approach here is to not introduce any breaking changes, as I don't know where and how these methods on d2.currentUser are used.

Perhaps we should include !users in the default fields filter?

That would be a breaking change.

[edoardo]

BTW, I just came to realise that for the specific problem of the interpretations panel, the group ids required are already in d2.currentUser just hidden behind a Symbol.
I'm going to add a new method getUserGroupIds which simply returns that list:

getUserGroupIds() { return this[propertySymbols.userGroups]; }

I will update the Interpretation component and replace d2.currentUser.getUserGroups() with d2.currentUser.getUserGroupIds() and all the request to userGroups should be gone from that component.

[HendrikThePendric]

My approach here is to not introduce any breaking changes, as I don't know where and how these methods on d2.currentUser are used.

Yeah, this was also why I didn't approve or request changes, I'm simply not sure what is best here.... Obviously not introducing breaking changes is a good thing. But in this case that actually means that everywhere we use this method without any fields as arg, we run the risk of crashing the server if there are many users.

[janhenrikoverland]

Looks like a good solution for now

[dhis2-bot]

🎉 This PR is included in version 31.9.1 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀