share modal: add groups tab by feature flag #2649

anikachurilova · 2024-04-16T14:29:49Z

closes access-requests: add UI tab for groups #2592

See most of the screenshots here inveniosoftware/invenio-communities#1127

Some error handlers:
On delete:

kpsherva · 2024-04-19T09:02:17Z

...sets/semantic-ui/js/invenio_app_rdm/landing_page/ShareOptions/AccessLinks/LinksSearchItem.js

-          onClick={handleDelete}
-          icon
-          labelPosition="left"
+      {error ? (


minor: could we discuss not using ternary operators with big blocks of code? it is a bit unreadable, an if statement even before the return statement would be probably better. Do we need the error message displayed inside the table row?

I put the message inside the row because it's an error that occurred on deleting a specific secret link. Do you mean we should put the error message on top of the modal?

ok I see, that was not obvious from the code, thanks for explaining.
What about ternary operators?

Replaced ternany operators with

{error && <ErrorMessage />} {!error && <... />}

I agree looks more readable now!

..._app_rdm/theme/assets/semantic-ui/js/invenio_app_rdm/landing_page/ShareOptions/ShareModal.js

kpsherva · 2024-04-19T09:12:05Z

...antic-ui/js/invenio_app_rdm/landing_page/ShareOptions/AccessUsersGroups/AccessUsersGroups.js

+        <Modal.Content className="share-content">
+          {loading && <Loader isLoading active />}
+          {!loading && results !== undefined && (
+            <UserGroupAccessSearchResult


understanding just from props, I have a feeling that this component has either too many responsibilities or is just a wrapper which is passing down props, could it be simplified somehow?
when we have too many responsibilities on one component, it becomes impossible to decouple functionalities and reuse the code.
Some of the props are redundant, for example record and recOwner, you can retrieve the owner inside the component using record. callbackEndpoint and endpoint would be potentially another example.
Some of the other props are just passed down from the parent component to UserGroupAccessSearchResult, maybe that would be an indicator to split this component and use some of the subcomponents directly here, without wrapping them with UserGroupAccessSearchResult.
Generally it looks to me there is some potential for simplification. The easiest would be to look for possible presentation vs data fetch components separation

kpsherva · 2024-04-19T09:14:18Z

...s/invenio_app_rdm/landing_page/ShareOptions/AccessUsersGroups/UserGroupAccessSearchResult.js

+    return (
+      <Table.Row>
+        {error ? (
+          <ErrorMessage


same comment as somewhere before: do we need the error message to be displayed inside the table?

kpsherva · 2024-04-19T09:23:42Z

..._app_rdm/theme/assets/semantic-ui/js/invenio_app_rdm/landing_page/ShareOptions/ShareModal.js

+
+    results.forEach((result) => {
+      if (result.id === id) {
+        result.permission = permission;


just for my understanding on how it works: I am not sure I fully understand why permission is set on the result, shouldn't the permission be already there?

This was done to update the list of results that won't require refetching the whole tab pane. As the only way to update the grant/link is to change a permission, that is why I am only updating it here.

kpsherva · 2024-04-19T09:24:55Z

..._app_rdm/theme/assets/semantic-ui/js/invenio_app_rdm/landing_page/ShareOptions/ShareModal.js

+      results = linksResults;
+    }
+
+    results.forEach((result) => {


if there are many results, could it have issues with performance?

I don't think we are expecting to have more than ~10-20 results in each tab, so I think this should be ok!

..._app_rdm/theme/assets/semantic-ui/js/invenio_app_rdm/landing_page/ShareOptions/ShareModal.js

kpsherva · 2024-04-19T09:32:55Z

..._app_rdm/theme/assets/semantic-ui/js/invenio_app_rdm/landing_page/ShareOptions/ShareModal.js

+    if (entityType === this.linksType) await this.fetchLinks(callbackEndpoint, true);
+  };
+
+  fetchUsersGroups = async (endpoint, entityType, isDataChanged) => {


general observation for this component: it has too many responsibilities, especially

fetching different types of data,

taking care of different tabs display

handles updates

I think it should be split into smaller components

Split the responsibilities between the components! Does it look better now?

...ssets/semantic-ui/js/invenio_app_rdm/landing_page/ShareOptions/AccessLinks/AccessDropdown.js

...ic-ui/js/invenio_app_rdm/landing_page/ShareOptions/AccessLinks/LinksSearchResultContainer.js

..._app_rdm/theme/assets/semantic-ui/js/invenio_app_rdm/landing_page/ShareOptions/ShareModal.js

ntarocco · 2024-04-19T15:33:42Z

..._app_rdm/theme/assets/semantic-ui/js/invenio_app_rdm/landing_page/ShareOptions/ShareModal.js

+    if (entityType === this.groupType) await this.fetchGroups(endpoint, isDataChanged);
+  };
+
+  fetchUsers = async (endpoint, isDataChanged) => {


I am not understanding the param isDataChanged: what is the purpose?

The purpose is to mark that data was added/deleted and the table of results needs refetching.
isDataChanged will be equal to false when clicking between tabs for more than 1 time. As a result the whole list is not being fetched every time user clicks on the tab.

..._app_rdm/theme/assets/semantic-ui/js/invenio_app_rdm/landing_page/ShareOptions/ShareModal.js

invenio_app_rdm/theme/assets/semantic-ui/less/invenio_app_rdm/theme/modules/modal.overrides

* closes inveniosoftware#2592

kpsherva · 2024-04-29T14:49:35Z

tests outcome (mainly UX improvements, can be implemented in a separate PR):

Active tab label should have brand colour (pinkish), otherwise it looks strange, like inactive tab
Owner should be counted in the number on the label, otherwise the numbers don't add up:
The permission level field does not stand out, therefore someone can just choose users and click "Add", which results in:

We need to either block the add button, until all required fields are filled or improve the error message, because it does not state which field is missing - the user won't associate permission with Access field
Search needs improvement, ticket already created Some users cannot be found when inviting or setting restrictions CERNDocumentServer/cds-rdm#114
When adding multiple users, the search works quite badly, it should favour users not yet added to the access grants:
( I was expecting to see user Test 2, which showed up only when typing the full name, otherwise the dropdown is taken over by the already added entries)
when clicking remove, we should have confirmation monit ("Are you sure you want to remove...?"), like everywhere else. The button should be red, since our actions are color-coded.
When clicking on remove, the whole modal goes to loading state, we should only display loading on the specific button, why to block all the rows, is there any specific reason? Otherwise the UI "jumps" due to unspecified minimal height while loader is displaying

* avoid fetching data of each tab pane on opening of the modal * fetch only the data of the opened pane * add missing error handlings

anikachurilova force-pushed the add-ui-tab-for-groups branch from 087542f to 5d7273a Compare April 16, 2024 14:41

kpsherva reviewed Apr 19, 2024

View reviewed changes

..._app_rdm/theme/assets/semantic-ui/js/invenio_app_rdm/landing_page/ShareOptions/ShareModal.js Outdated Show resolved Hide resolved

kpsherva reviewed Apr 19, 2024

View reviewed changes

..._app_rdm/theme/assets/semantic-ui/js/invenio_app_rdm/landing_page/ShareOptions/ShareModal.js Outdated Show resolved Hide resolved

kpsherva reviewed Apr 19, 2024

View reviewed changes

..._app_rdm/theme/assets/semantic-ui/js/invenio_app_rdm/landing_page/ShareOptions/ShareModal.js Outdated Show resolved Hide resolved

kpsherva reviewed Apr 19, 2024

View reviewed changes

..._app_rdm/theme/assets/semantic-ui/js/invenio_app_rdm/landing_page/ShareOptions/ShareModal.js Outdated Show resolved Hide resolved

kpsherva reviewed Apr 19, 2024

View reviewed changes

ntarocco reviewed Apr 19, 2024

View reviewed changes

anikachurilova force-pushed the add-ui-tab-for-groups branch 2 times, most recently from cd4747d to 95cbc58 Compare April 24, 2024 08:25

share modal: add groups tab by feature flag

73493ae

* closes inveniosoftware#2592

anikachurilova force-pushed the add-ui-tab-for-groups branch 5 times, most recently from adbc3fb to e3d3aff Compare April 26, 2024 06:52

share tab: improve performance

9e1e6a0

* avoid fetching data of each tab pane on opening of the modal * fetch only the data of the opened pane * add missing error handlings

anikachurilova force-pushed the add-ui-tab-for-groups branch from e3d3aff to 9e1e6a0 Compare May 7, 2024 09:58

ntarocco merged commit 8d13662 into inveniosoftware:master May 7, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

share modal: add groups tab by feature flag #2649

share modal: add groups tab by feature flag #2649

anikachurilova commented Apr 16, 2024 •

edited

Loading

kpsherva Apr 19, 2024 •

edited

Loading

anikachurilova Apr 19, 2024

kpsherva Apr 25, 2024

anikachurilova Apr 26, 2024

kpsherva Apr 19, 2024 •

edited

Loading

kpsherva Apr 19, 2024

kpsherva Apr 19, 2024

anikachurilova Apr 22, 2024

kpsherva Apr 19, 2024

anikachurilova Apr 22, 2024

kpsherva Apr 19, 2024 •

edited

Loading

anikachurilova Apr 24, 2024

ntarocco Apr 19, 2024

anikachurilova Apr 22, 2024

kpsherva commented Apr 29, 2024

share modal: add groups tab by feature flag #2649

share modal: add groups tab by feature flag #2649

Conversation

anikachurilova commented Apr 16, 2024 • edited Loading

kpsherva Apr 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kpsherva Apr 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kpsherva Apr 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kpsherva commented Apr 29, 2024

anikachurilova commented Apr 16, 2024 •

edited

Loading

kpsherva Apr 19, 2024 •

edited

Loading

kpsherva Apr 19, 2024 •

edited

Loading

kpsherva Apr 19, 2024 •

edited

Loading