Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Broken UI when defining a group who may use the app #526

Closed
ostasevych opened this issue Dec 3, 2023 · 4 comments
Closed

Broken UI when defining a group who may use the app #526

ostasevych opened this issue Dec 3, 2023 · 4 comments
Labels
1. to develop Accepted and waiting to be taken care of bug Something isn't working feature: settings good first issue Good for newcomers

Comments

@ostasevych
Copy link

The webUI which allows to select which groups may use the app is broken: after defining the group it doesn't show it, but the empty field, and after the reload the field disappears.

  1. Select group of users
    image

  2. Click Save

  3. Reload page

  4. Observe empty field
    image

  5. Click Save

  6. Observe undefined state without any field at all
    image
@ostasevych ostasevych added 0. Needs triage Pending approval or rejection. This issue is pending approval. bug Something isn't working labels Dec 3, 2023
@joshtrichards joshtrichards added feature: settings 1. to develop Accepted and waiting to be taken care of and removed 0. Needs triage Pending approval or rejection. This issue is pending approval. labels Dec 27, 2023
@joshtrichards joshtrichards added the good first issue Good for newcomers label Jan 5, 2024
@mawumag
Copy link
Contributor

mawumag commented Jan 9, 2024

I can reproduce the bug. More in detail:

  1. On first "Save" (using admin as input as in the pictures above):
    • The UI correctly updates the oc_appconfig table in the database.
    • The value for the key allowed_groups is now ["admin"].
  2. On "Refresh":
    • The database entry is correctly queried and registered in an #initial-state- element by nextcloud (base64 encoded).
    • The UI seems to be unable to correctly display the groups, it only shows the weird box as per second screenshot by OP.
  3. On second "Save":
    • The UI updates the allowed_groups property to [null], which leads to a broken UI. If admin is (re)added before saving, it will be updated to [null,"admin"], also resulting in a broken UI.

Some additional observations:

  • I tried with different groups (such as test) and the results are the same.
  • If one initially tries to select two groups, say admin and test, the allowed_groups property is written to the database as ["admin","test"] as expected, but the UI seems to be unable to load it at all (spinning wheel in the multiselect box is displayed forever).

Given the above, I suspect the issue lies in the src/components/AdminSection.vue logic, but I have not been able to find anything wrong with it so far.

@mawumag
Copy link
Contributor

mawumag commented Jan 11, 2024

UPDATE:
I also verified that the component correctly pulls the list of allowed groups from the database, and the variables groups and allowedGroups are correctly assigned.
I can only imagine that the component NcMultiselect is not working properly. Indeed, it seems to have been deprecated (see for example nextcloud/server#38145, nextcloud/server#40588).

I have switched NcMultiselect to NcSelect as suggested in the issues linked above, and everything seems to work properly (it also looks better in my opinion).

I can provide a pull request shortly after some more testing.

mawumag added a commit to mawumag/end_to_end_encryption that referenced this issue Jan 11, 2024
@mawumag
Deprecated NcMultiselect replaced by NcSelect - fixes nextcloud#526
2214908 
- The UI in the admin section for setting allowed groups is broken, see nextcloud#526.
- Replacing the deprecated NcMultiselect component with NcSelect (as per recommendations) fixes the issue.
@mawumag mawumag mentioned this issue Jan 11, 2024
Closed
mawumag added a commit to mawumag/end_to_end_encryption that referenced this issue Jan 12, 2024
@mawumag
Deprecated NcMultiselect replaced by NcSelect - fixes nextcloud#526
54959a7 
- The UI in the admin section for setting allowed groups is broken, see nextcloud#526.
- Replacing the deprecated NcMultiselect component with NcSelect (as per recommendations) fixes the issue.

Signed-off-by: Marco Baggio <70693636+mawumag@users.noreply.github.com>
mawumag added a commit to mawumag/end_to_end_encryption that referenced this issue Jan 13, 2024
@mawumag
allowed_groups fetched from db JSONified - fixes nextcloud#526
41fb5fe 
- The UI in the admin section for selecting allowed groups is broken, see nextcloud#526
- The allowed_groups fetched from the database are in a different format compared to the one expected by the UI logic. This commit fixes this by reconstructing a JSON object with the appropriate attributes for each fetched allowed group.

Signed-off-by: Marco Baggio <70693636+mawumag@users.noreply.github.com>
mawumag added a commit to mawumag/end_to_end_encryption that referenced this issue Jan 13, 2024
@mawumag
allowed_groups fetched from db JSONified - fixes nextcloud#526
57d0c8b 
- The UI in the admin section for selecting allowed groups is broken, see nextcloud#526
- The allowed_groups fetched from the database are in a different format compared to the one expected by the UI logic. This commit fixes this by reconstructing a JSON object with the appropriate attributes for each fetched allowed group.

Signed-off-by: Marco Baggio <70693636+mawumag@users.noreply.github.com>
mawumag added a commit to mawumag/end_to_end_encryption that referenced this issue Jan 13, 2024
@mawumag
allowed_groups fetched from db JSONified - fixes nextcloud#526
557fb4f 
- The UI in the admin section for selecting allowed groups is broken, see nextcloud#526
- The allowed_groups fetched from the database are in a different format compared to the one expected by the UI logic. This commit fixes this by reconstructing a JSON object with the appropriate attributes for each fetched allowed group.

Signed-off-by: Marco Baggio <70693636+mawumag@users.noreply.github.com>
@mawumag mawumag mentioned this issue Jan 13, 2024
Merged
artonge added a commit that referenced this issue Jan 17, 2024
@artonge
Merge pull request #546 from mawumag/bugfix/broken-allowed-groups-ui
b513498 
allowed_groups fetched from db JSONified - fixes #526
@joshtrichards
Copy link
Member

Thanks @mawumag for looking into and taking care of this!

@mawumag
Copy link
Contributor

mawumag commented Jan 17, 2024

My pleasure! It might be that some users have a database entry for allowed_groups that is inconsistent (e.g. [null]) due to the previous behaviour.

I have not tested what happens with the new UI in this case, but it might be necessary to implement a fix if users report issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
1. to develop Accepted and waiting to be taken care of bug Something isn't working feature: settings good first issue Good for newcomers
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@joshtrichards @ostasevych @mawumag