fossology / FOSSologyUI

Repository to hold the new UI framework for FOSSology built with React
https://fossology.github.io/FOSSologyUI/
GNU General Public License v2.0
49 stars 88 forks source link

feat(user-group): display user group in dropdown #104

Closed Aman-Codes closed 3 years ago

Aman-Codes commented 3 years ago

Description

Created user group dropdown. Closes #90

Changes

Screenshots

image image

How to test

Check header

shaheemazmalmmd commented 3 years ago

Some how i'm not able to see the groups. screenShot19

Aman-Codes commented 3 years ago

Hi @shaheemazmalmmd Could you please try to logout and login again to see what is the response from the /groups API. Please do share the response from that request and the corresponding saved valued in local storage. Something like this: image image

shaheemazmalmmd commented 3 years ago

Ah. logout and login works . Thanks !

GMishx commented 3 years ago

Will it be good a check, if groups does not exists in local store, fetch it from /groups and then continue loading the UI?

Aman-Codes commented 3 years ago

Will it be good a check, if groups does not exists in local store, fetch it from /groups and then continue loading the UI?

@GMishx Yes, It will help the user to prevent logging in again if they clear their local storage but it will also add a overhead on each page reload. Should I add such a check on private and admin layout component?

Aman-Codes commented 3 years ago

If the group data does not exist in local storage and get request to /groups fails even after multiple retries then we would need to logout the user. So updated the PR to logout user incase of missing data in local storage as discussed in the yesterday meeting with @shaheemazmalmmd

GMishx commented 3 years ago

Just tested the branch, have following feedback:

  1. I kept the user in local storage and token cookie and removed the groups from storage. Reloading the page took me to login but I still had token and user as is. It this expected?
  2. Selecting any group does not change the icon, it keeps showing "GN".
  3. Selecting any group appears to be not storing the selection and thus any consecutive call to API does not have proper groupName header. Are you working on this?
Aman-Codes commented 3 years ago

Hi @GMishx thanks for the review! Just few queries

  1. I kept the user in local storage and token cookie and removed the groups from storage. Reloading the page took me to login but I still had token and user as is. It this expected?

Yes, Currently it was kept intentionally. Could you please tell which approach seems better to you?

  1. Incase of valid cookie but missing group info, make get request to /groups but if request fails retry for say 10 times (logout if it still fails).
  2. Or the Current one, i.e., logout if missing group info?
  1. Selecting any group does not change the icon, it keeps showing "GN".

Currently the icon only shows "GN" as there is no available API to return the default group on login. Created an issue #2054 in fossology for it.

  1. Selecting any group appears to be not storing the selection and thus any consecutive call to API does not have proper groupName header. Are you working on this?

Yes I was planning to create another pull request for this as we need to discuss some edge cases. Currently all API request accepts group name header except /users, /users/{id}, /users/self, /jobs/{id}. So should I add groupName header to all API request by modifying sendRequest.js file or provide the header in only those request which need it? Also for job schedule and upload analysis page the groupName is taken from their form and not from the navbar dropdown. Should I create a separate pull request for this or add in this one?

GMishx commented 3 years ago

Yes, Currently it was kept intentionally. Could you please tell which approach seems better to you?

1. Incase of valid cookie but missing group info, make get request to /groups but if request fails retry for say 10 times (logout if it still fails).

2. Or the Current one, i.e., logout if missing group info?

The current approach is good. But I was expecting the cookie and local storage to be gone. It can become a security vulnerability.

  1. Selecting any group does not change the icon, it keeps showing "GN".

Currently the icon only shows "GN" as there is no available API to return the default group on login. Created an issue #2054 in fossology for it.

On initial load, it is fine. But when I select a group from drop-down, isn't it supposed to change?

  1. Selecting any group appears to be not storing the selection and thus any consecutive call to API does not have proper groupName header. Are you working on this?

Yes I was planning to create another pull request for this as we need to discuss some edge cases. Currently all API request accepts group name header except /users, /users/{id}, /users/self, /jobs/{id}. So should I add groupName header to all API request by modifying sendRequest.js file or provide the header in only those request which need it? Also for job schedule and upload analysis page the groupName is taken from their form and not from the navbar dropdown.

Currently all endpoints accept the groupName in header (some might be missing it in documentation). So it is absolutely safe to add it to every request in sendRequest.js. And for job schedule and upload analysis APIs, they can still accept the groupName and have an override as the request body. Like for reuse agent, you can even reuse decision from different group than to which you are uploading file.

Should I create a separate pull request for this or add in this one?

Whatever is fine with you.

Aman-Codes commented 3 years ago

Just tested the branch, have following feedback:

  1. I kept the user in local storage and token cookie and removed the groups from storage. Reloading the page took me to login but I still had token and user as is. It this expected?
  2. Selecting any group does not change the icon, it keeps showing "GN".
  3. Selecting any group appears to be not storing the selection and thus any consecutive call to API does not have proper groupName header. Are you working on this?

@GMishx Updated the PR to handle all the above mentioned points

GMishx commented 3 years ago

Even when logged out, the groups button is visible.

image

@Aman-Codes if you can please fix this too, I can merge this branch.

Aman-Codes commented 3 years ago

@GMishx Updated it