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.

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

feat: add data sharing to sharing dialog [LIBS-677] #1629

Merged
merged 8 commits into from
Nov 13, 2024

Conversation

tomzemp
Copy link
Member

@tomzemp tomzemp commented Nov 11, 2024

Implements LIBS-677


Description

Adds data sharing to sharing dialog:

image

If you do not have dataSharing: true, the sharing dialog continues to behave as before (there's probably some slight css differences from previous, but nothing of particular note):
image


Some notes:

  • to enable data sharing in sharingDialog, you have to set dataSharing: true on component. By default, this is false, and hence if not provided, you will just see a sharing dialog for metadata. @cooper-joe and I discussed whether we should try to either automatically enable data sharing for certain object types or automatically prevent if for certain object types, and we thought that that would create a fair amount of work to check/maintain lists of relevant objects for data sharing. In the end, we decided to just let consumers decide if they should enable data sharing.
  • For backwards compatibility, I've kept the initialSharingSettings on the component to allow the passing of ACCESS_NONE, VIEW_ONLY, or VIEW_AND_EDIT (rather than the object format of {data: ACCESS_NONE | VIEW_ONLY | VIEW_AND_EDIT, metadata: ACCESS_NONE | VIEW_ONLY | VIEW_AND_EDIT}. If you provide access details as a string, this is assumed to apply for metadata only, e.g. passing access: "VIEW_ONLY" will be converted to access: {data: "NO_ACCESS", metadata: "VIEW_ONLY"}
  • @cooper-joe and I had some discussion around how the "remove access" option should work. We decided that the simplest UX for now (e.g. the one that lets us edit the existing component without a design rethink), was to present the "remove access" option only if the other sharing field has been set to "No access". E.g. if you have a user and you set metadata to "No access", then you will see "remove access" as an option in the data sharing dropdown (the idea here is that you cannot remove access until you are at a point where the other field already does not have access). We also decided that you can explicitly set metadata:NO_ACCESS, data:NO_ACCESS if you choose (that's allowed/persisted by the backend), so we did not want to make the UX behave differently.
image
  • as part of this work, I discussed with @cooper-joe the descriptions that we were using under the user/group/all users name. Previously, we were just summarizing the access again, e.g
image The specs on the ticket had an update to explain the actual user/group/all users in more detail (simply repeating the access information looked particularly odd when there was both metadata/data access), so I've updated to do that (with note that I'm using user.id, rather than user.username for now): image

Known issues

  • According to the design specs, the details text for a user should be the username. This is not, however, being provided by the api/sharing response, and making a request just to retrieve username seems inefficient. Talked this over with Joe and we decided that we would use user.id for now and request that username be added to api/sharing response for users array.
  • there is a risk of race conditions (this already exists in the old sharing dialog as you could change the sharing twice and the first attempt might post later, but I suppose the race condition probability increases when you have the chance to change both data/metadata sharing in the edit operations). Joe and I discussed an alternative that would prevent this (making users commit their edit operations by clicking on a separate button), but decided for simplicity's sake to keep the existing UX and assume that most people will not be affected by race condition problem

Checklist

  • API docs are generated
  • Tests were added
  • Storybook demos were added

Note: I've overwritten the automatically generated API docs as they were not properly generating a description for the initialSharingSettings object. (This might be counterproductive, as I suppose it will be overwritten)

For tests, I've added a number of e2e tests in cypress. The existing tests cover the component in metadata only mode, so I've added a new feature file that covers the add/edit/remove functionality with data sharing. The tests could have more iterations in the scenarios, but I didn't think it was particularly useful to add more (given the comparative slowness of the cypress tests + lack of extra value of the additional scenarios)

@tomzemp tomzemp requested a review from a team as a code owner November 11, 2024 10:02
@dhis2-bot
Copy link
Contributor

dhis2-bot commented Nov 11, 2024

🚀 Deployed on https://pr-1629--dhis2-ui.netlify.app

@dhis2-bot dhis2-bot temporarily deployed to netlify November 11, 2024 10:16 Inactive
Copy link
Collaborator

@kabaros kabaros left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks great - all the new tests make it easier to understand the effect of the changes. Only thing missing from my point of view are the TS defintion updates.

}

.select-wrapper {
flex: 1;
}
.leftWrapperData {
Copy link
Collaborator

@kabaros kabaros Nov 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: I guess the names of the classes can also be "logical" (start rather than left etc..)

`}</style>
</>
)
}

AccessAdd.propTypes = {
onAdd: PropTypes.func.isRequired,
dataSharing: PropTypes.bool,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

types also need updating?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the reminder! I've cleaned up the type definition, which also includes some fixes:

  • I've added in dataSharing on the component level
  • I've updated the users/groups to be arrays of SharingObject
  • I've replaced the import of ModalOnCloseEventHandler with the definition from ui/modal types (the type was exported from modal types and only consumed in this type file, but was imported incorrectly)

I spoke with @Birkbjo and we decided that for the access type, we would just use the new type definition {metadata: 'ACCESS_NONE' | 'VIEW_ONLY' | 'VIEW_AND_EDIT'; data: 'ACCESS_NONE' | 'VIEW_ONLY' | 'VIEW_AND_EDIT'} rather than also advertise the existence of the backwards compatible type. (Note: that none of our DHIS2 repos appear to currently be using the initialSharingSettings prop when consuming component)

}
)

// dhis2-uicore-singleselect-prefix
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unnecessary comment

@dhis2-bot dhis2-bot temporarily deployed to netlify November 12, 2024 12:50 Inactive
@tomzemp tomzemp merged commit 7e15c7f into master Nov 13, 2024
16 checks passed
@tomzemp tomzemp deleted the LIBS-677/data-sharing branch November 13, 2024 08:19
dhis2-bot added a commit that referenced this pull request Nov 13, 2024
# [9.14.0](v9.13.0...v9.14.0) (2024-11-13)

### Features

* add data sharing to sharing dialog [LIBS-677] ([#1629](#1629)) ([7e15c7f](7e15c7f))
@dhis2-bot
Copy link
Contributor

🎉 This PR is included in version 9.14.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

dhis2-bot added a commit that referenced this pull request Nov 21, 2024
# [10.0.0-alpha.8](v10.0.0-alpha.7...v10.0.0-alpha.8) (2024-11-21)

### Bug Fixes

* peer dependency issue with npm publish ([#1628](#1628)) ([1319654](1319654))
* update calendar tests for react 18 ([98831a7](98831a7))
* update testing-library for selector-bar ([893024d](893024d))
* **translations:** sync translations from transifex (master) ([d89ce94](d89ce94))
* **translations:** sync translations from transifex (master) ([7f22330](7f22330))

### Features

* add data sharing to sharing dialog [LIBS-677] ([#1629](#1629)) ([7e15c7f](7e15c7f))
* make input field clearable and add prefix icon ([#1619](#1619)) ([7f87fb4](7f87fb4))
* update react peer dependency to react@18 ([#1624](#1624)) ([5d3c2a4](5d3c2a4))
@dhis2-bot
Copy link
Contributor

🎉 This PR is included in version 10.0.0-alpha.8 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants