feat: add share reason in share requests #498

noah-paige · 2023-06-06T14:13:49Z

Feature or Bugfix

Feature

Detail

Expose the Share Request Comments as Part of the Share View for the Approver to See
Allow Approver to Provide Comments when Rejecting a Share for the Requester to See

Relates

[ Show reason to approver why share is requested by the consumer and vice versa #422 ]

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

noah-paige · 2023-06-06T15:49:31Z

This PR adds the Share Request Purpose specified when requesting data access to the Share View so that the Approver of the share request is able to understand why the requester needs access

Additionally, this PR adds the ability of the approver to add the reject purpose when they reject a share request so the requester is able to see why the request was rejected

backend/dataall/db/models/ShareObject.py

frontend/src/views/Shares/ShareView.js

dlpzx

in general it looks good

noah-paige · 2023-06-07T13:51:13Z

I added string limit to the reject purpose similar to that of the request purpose we have in place using Yup.string().max(5000)

…r role

noah-paige · 2023-06-09T15:08:10Z

Finished writing unit tests - ready for another review

backend/migrations/versions/72b8a90b6ee8__share_request_purpose.py

backend/dataall/db/api/share_object.py

backend/dataall/api/Objects/ShareObject/resolvers.py

dlpzx · 2023-06-13T13:13:34Z

I just checked the following:

Add request purpose at request creation
Add reject purpose at rejection
Add either of the above when editing the request (checking approvers and requesters personas
Leave everything empty and check that it does not break anything
Edit either of the purposes and check that they get updated

For the frontend I think we could have something a bit more organised. On the right we have the request details and on the left the dataset details. I would place the request purpose on the request details or in a different box like in this screenshot:

I did some testing in 9c75ac1, if you want (only if you want) you can take it as reference

noah-paige · 2023-06-13T13:18:31Z

I like this frontend design screenshot better - let me add that separate box and get a new commit up

noah-paige · 2023-06-13T13:34:57Z

Latest Commit:

Removes print statement
Fix Typo in Migration Script
Change Share Object Comments Frontend View to a new box as shown in @dlpzx screenshot above

backend/dataall/api/Objects/ShareObject/resolvers.py

dlpzx · 2023-06-21T14:46:11Z

backend/migrations/versions/72b8a90b6ee8__share_request_purpose.py

+
+
+def downgrade():
+    # ### commands auto generated by Alembic - please adjust! ###


We are missing the back-filling when downgrading. I know, we hardly ever downgrade, but once I had a customer that needed to downgrade and it was crucial that migrations were both-sided

I just updated the PR with code for downgrading and did tests on both upgrade and downgrade again on the Aurora DB

Validated the resource permissions and columns got upgraded and downgraded as expected. @dlpzx - Think we are good here if you want to do one last final review

dlpzx · 2023-06-21T14:46:45Z

backend/dataall/db/api/share_object.py

@@ -1031,6 +1063,7 @@ def list_user_received_share_requests(
                        f'{{{username}}}'
                    ),
                    models.Dataset.stewards.in_(groups),
+                    models.Dataset.SamlAdminGroupName.in_(groups),
                )


good catch!

frontend/src/views/Shares/ShareView.js

tests/api/conftest.py

dlpzx

final final details

dlpzx · 2023-06-22T06:39:27Z

frontend/src/views/Shares/ShareView.js

@@ -764,6 +765,7 @@ const ShareView = () => {
                        <Typography
                          color="textPrimary"
                          variant="subtitle2"
+                          sx={{ wordBreak: "break-word" }}


dlpzx

Looks good, thanks for including so many small fixes in this PR. I also re-tested the migration up and down an it is 🥇

### Feature or Bugfix Release PR with the following list of features. Refer to each PR for the details ### Detail - #498 - #482 - #543 - #524 (which also solves #531) - #532 - #535 - #497 - #515 - #529 - #562 - #455 - #572 - #567 - #573 - #579 - #578 - #582 ### Breaking changes - release notes - ⚠️ IMPORTANT: upgrade to a version >V1.5.0 before upgrading to V1.6 to avoid deletion of resources in custom resource deletion - ⚠️ IMPORTANT: requires an update of environments and then datasets after upgrading. Either using cdk.json parameter `enable_update_dataall_stacks_in_cicd_pipeline`, waiting for overnight update stack task, or manually updating first environments and then datasets - CloudFront distribution replace for #529 - Additional EC2 permissions in CDK Synth CodeBuild stage for #543 --> this can be avoided by upgrading to v1.5.6 before upgrading to v1.6.0 - local development affected by more restrictive pivotRole trust policy ### Relates V1.6.0 release By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. --------- Co-authored-by: Gezim Musliaj <102723839+gmuslia@users.noreply.github.com> Co-authored-by: Noah Paige <69586985+noah-paige@users.noreply.github.com> Co-authored-by: nikpodsh <124577300+nikpodsh@users.noreply.github.com> Co-authored-by: chamcca <40579012+chamcca@users.noreply.github.com> Co-authored-by: Nikita Podshivalov <nikpodsh@amazon.com> Co-authored-by: dbalintx <132444646+dbalintx@users.noreply.github.com> Co-authored-by: mourya-33 <134511711+mourya-33@users.noreply.github.com>

noah-paige added 2 commits June 5, 2023 16:24

Add Share Request Reason and Reject Reason

ba50610

Tweaks Reject Share Object Modal

fce6d68

noah-paige linked an issue Jun 6, 2023 that may be closed by this pull request

Show reason to approver why share is requested by the consumer and vice versa #422

Closed

noah-paige added 2 commits June 6, 2023 10:16

linting

8955c1d

linting

9eca7ff

dlpzx changed the title ~~422 add share reason~~ feat: add share reason in share requests Jun 6, 2023

noah-paige added 2 commits June 6, 2023 11:31

Unit Tests

1eed926

unit test

c92af38

noah-paige changed the base branch from main to v1m6m0 June 6, 2023 15:41

noah-paige marked this pull request as ready for review June 6, 2023 15:49

dlpzx reviewed Jun 7, 2023

View reviewed changes

backend/dataall/db/models/ShareObject.py Show resolved Hide resolved

dlpzx reviewed Jun 7, 2023

View reviewed changes

frontend/src/views/Shares/ShareView.js Show resolved Hide resolved

dlpzx reviewed Jun 7, 2023

View reviewed changes

noah-paige added 2 commits June 7, 2023 09:48

Add String Limit Reject Share

df794b3

Remove commented out code

ba76e79

noah-paige added 4 commits June 8, 2023 16:13

Add Edit ability of Share request and reject purpose depending on use…

236568d

…r role

lint

72b6960

Add unit tests and fix string limits

42aa8bc

Finish unit tests

bb4a618

Clear reject purpose on approve

826dbc5

dlpzx reviewed Jun 13, 2023

View reviewed changes

backend/migrations/versions/72b8a90b6ee8__share_request_purpose.py Outdated Show resolved Hide resolved

dlpzx reviewed Jun 13, 2023

View reviewed changes

backend/dataall/db/api/share_object.py Show resolved Hide resolved

dlpzx reviewed Jun 13, 2023

View reviewed changes

backend/dataall/api/Objects/ShareObject/resolvers.py Outdated Show resolved Hide resolved

dlpzx reviewed Jun 13, 2023

View reviewed changes

backend/dataall/api/Objects/ShareObject/resolvers.py Show resolved Hide resolved

Fix typo and edit frontend view for share comments

4f07c26

noah-paige added 5 commits June 14, 2023 16:27

Update Share Object Permissions for SUBMIT_SHARE_OBJECT

6ff5017

Transfer Stewardship

7302fb6

Remove commented out code

cfabeb4

Dataset Admins get Approver Share Object Status

542bd6e

linting

2066f90

dlpzx reviewed Jun 21, 2023

View reviewed changes

backend/dataall/api/Objects/ShareObject/resolvers.py Outdated Show resolved Hide resolved

dlpzx reviewed Jun 21, 2023

View reviewed changes

frontend/src/views/Shares/ShareView.js Show resolved Hide resolved

dlpzx reviewed Jun 21, 2023

View reviewed changes

tests/api/conftest.py Outdated Show resolved Hide resolved

dlpzx reviewed Jun 21, 2023

View reviewed changes

noah-paige added 5 commits June 21, 2023 12:43

clean up commented out code in conftest

76e92ad

Wrap Lines for Reject Request Purposes

6d339d2

Fix resolve_user_role() for consumption roles

0f44b18

Add Downgrade Share Object Permissions Revert

e77015b

Testing Downgrade Fix

8ddd6dd

dlpzx self-requested a review June 22, 2023 06:38

dlpzx reviewed Jun 22, 2023

View reviewed changes

dlpzx approved these changes Jun 22, 2023

View reviewed changes

noah-paige merged commit 4aff9a7 into v1m6m0 Jun 22, 2023

dlpzx mentioned this pull request Jul 11, 2023

V1.6.0 features #565

Merged

dlpzx deleted the 422-add-share-reason branch July 20, 2023 09:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add share reason in share requests #498

feat: add share reason in share requests #498

noah-paige commented Jun 6, 2023

noah-paige commented Jun 6, 2023

dlpzx left a comment

noah-paige commented Jun 7, 2023

noah-paige commented Jun 9, 2023

dlpzx commented Jun 13, 2023 •

edited

Loading

noah-paige commented Jun 13, 2023

noah-paige commented Jun 13, 2023

dlpzx Jun 21, 2023

noah-paige Jun 21, 2023

dlpzx Jun 21, 2023

dlpzx left a comment

dlpzx Jun 22, 2023

dlpzx left a comment



		def downgrade():
		# ### commands auto generated by Alembic - please adjust! ###

feat: add share reason in share requests #498

feat: add share reason in share requests #498

Conversation

noah-paige commented Jun 6, 2023

Feature or Bugfix

Detail

Relates

noah-paige commented Jun 6, 2023

dlpzx left a comment

Choose a reason for hiding this comment

noah-paige commented Jun 7, 2023

noah-paige commented Jun 9, 2023

dlpzx commented Jun 13, 2023 • edited Loading

noah-paige commented Jun 13, 2023

noah-paige commented Jun 13, 2023

dlpzx Jun 21, 2023

Choose a reason for hiding this comment

noah-paige Jun 21, 2023

Choose a reason for hiding this comment

dlpzx Jun 21, 2023

Choose a reason for hiding this comment

dlpzx left a comment

Choose a reason for hiding this comment

dlpzx Jun 22, 2023

Choose a reason for hiding this comment

dlpzx left a comment

Choose a reason for hiding this comment

dlpzx commented Jun 13, 2023 •

edited

Loading