Open huss opened 7 months ago
Hi, We want to take this issue.
@BigGeF Thanks for looking into this and I've assigned you the task. Please let me know if I can help. As stated, there may be other/better ways to address this.
@huss Question 1 I registered three non-admin accounts (CSV, Export, and Obvius user), but the "units" option is not appearing in the dropdown menu. Should we make the "units" option visible in the dropdown, or should we first focus on resolving the issue by displaying values in Groups/meters? What kind of value needs to be shown? Question 2 What kind of values needs to be shown? where do they need to be shown?
Is this the correct place that you want to show more details for each "Card"?
Is this the correct place that you want to show more details for each "Card"?
@BigGeF Thank you for asking and giving me the opportunity to clarify the issue. I think there are two aspect of the user login that should be differentiated:
With that clarification, lets look at the Redux state that is central to this issue. This can be seen by looking at the state definitions in the code but is probably easier, esp. for this issue, via the Redux developer tool extension for a web browser. If one inspects the page and then chooses the Redux option then the Redux state is shown. Lets start with meters since they are a better example. If you are a non-admin user then looking at the meter state will show something similar to this:
Notice that many meter Redux state items are null so are not available to non-admins. Not all meter Redux state is shown in this picture. Some meter values are in Redux state as the actual value since non-admins must have access to them. If you redo this when an admin user you will see:
Note the id is still 1 so it is the same meter. Now most of the null information an actual value. Some values are actually null (e.g., url) so do not change. Thus, it is the null (I think that is the only special value but need to verify) that is causing the issue in the code when it is looked at for non-admins. Logging in/out should automatically change the Redux state to the desired values.
Now doing the same for units as a non-admin gives:
In this case all the values are stored in Redux state. It isn't clear that is the best choice. For example, the note should not be used (or probably seen) by non-admin users. This isn't the issue here but probably shows that once it is resolved to make hiding the state easier from a coding standpoint then OED should hide those values. For now, it can be ignored.
Thus, the issue is to try to figure a better way of avoiding errors when the state is hidden than optional chaining. OED would prefer to leave the values as null to make it clear they are hidden (vs. using a special value). That would be reconsidered if it seems the only solution but then all uses need to be checked to make sure the dummy values are not accessed and no null checks were made (to detect non-admins) that need to be updated.
I hope this helps and makes sense. Please ask about anything else or if we can help in some way.
Thank you for your detailed explanation. I still have a question. Does the CSV user count as a non-admin user? This screenshot shows the data I received using the CSV role, and it looks the same as with the admin role.
register a CSV
Good question. Because a CSV user can upload a meter and also create them via the CSV readings page, they can modify a meter (and thus really see the meter values). Given this, OED decided that a CSV user would see the meter info in Redux state. (I cannot recall if it was required for some actions but it may be.) I don't recall about the other state but it is okay if a CSV user sees it.
Is your feature request related to a problem? Please describe.
Some Redux state for meters/groups/units are set to null/special values in the route/Redux so non-admins cannot see the values. This can cause issues when the state is accessed. Optional chaining is currently used in a number of places because TypeScript does not like this because the definitions do not allow for undefined/null.
Describe the solution you'd like
A better solution would be for TypeScript to understand the allowed values. This would allow for strict typing as desired.
One thought is to put in two routes: admin & non-admin with different types. These could then be used to define TypeScript types allowed overall. This is not carefully thought out and other ideas should be considered.
Describe alternatives you've considered
Continue to use optional chaining to fix each place this comes up but it probably won't catch them all and does not allow TypeScript to do full checking.
Additional context
None