-
Notifications
You must be signed in to change notification settings - Fork 82
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
ShareView remake #1277
ShareView remake #1277
Conversation
@SofiaSazonova @dlpzx @noah-paige I think perhaps this is an improvement but I think we could do better... I think draft shares should not exist at all. A share either exists as submitted or doesn't exist at all. I don't see any value in draft shares and I think they they just serve to add confusion. I think we should rework the graphql endpoints to allow to submit a share together with all the items selected. This would be more work and you couldn't reuse likely the existing UI components but it solves a lot of problems we're trying to work around: putting reminders in red text, adding more modals and buttons to click. I see this modal + red text as bandaging over symptoms vs solving the core problem. if you really insist to keep drafts:
|
I agree with Zi. It would be preferable not to encounter an additional page after navigating through the request access modal. I suggest that upon clicking the lock button on a dataset, the user should be directed either to a request access modal or a share page. Here, they should be prompted to fill out all the details from the original request access modal along with the current share page options, such as selecting share items. Having two pages solely for submitting a share request seems unnecessary. Additionally, if we opt to retain the draft stage page, we should implement the steps outlined by Zi. I particularly emphasize step 7, where instead of a submit button, we should display a "Create Draft Share" button. Subsequently, upon someone clicking on this button, a red pop-up notification should appear at the top right, indicating that the draft request has been submitted and prompting the user to complete the rest of the form to finalize their share request." |
Hi @SofiaSazonova , the sleek UI looks nicer than the earlier one. Adding to the comments from @anushka-singh and @zsaltys ,
|
@zsaltys @anushka-singh @TejasRGitHub Thanks for the feedback! |
In addition to the above comments:
|
|
Long role names will be croped by the width of the line and "..." will be added. Full name will be available in the tooltip.
"Revoke" is now shown only in Edit window. Verify and Reapply are still useful in drafts: share could be previously approved and processed. Then, it was drafted again, but previously shared items still can be diagnosed and reapplied if needed.
There are some 'backend strings' that I would love to rename as well, but I will keep for separate PR
Good Idea
Now the share can be submitted as soon as it's edited, so usually the user won't have to use this modal. I keep it as it is for now.
Now the status is shown, because we have one editing table for all items
Good Idea
I keep it now in both places, for i don't want to break the already established ways users can be accustomed to.
In current workflow, user will see right away, what is selected. |
Now, it's there
It's not really a UI change, but more the workflow change as well. We can discuss it separately and create a separate issue
Now we can do the whole share submission from catalog page in modal window.
Good suggestion. I don't want to overcomplicate this PR, but this option definitely needs some clarification in UI |
Hi @SofiaSazonova - think we are getting there but found a couple issues when playing around with the UI and testing locally: [1] Does this work for Dashboard Shares via requestAccessModal? [2] If already have a share and open a share request from catalog for another item via RequestAccessModal
[3] When I have a share in submitted state with 1+ items
[4] From ShareEditForm opened via ShareView
[5] From ShareEditForm opened via ShareView
[6] From ShareEditForm opened via ShareView
|
About this
It actually the way it was before. If share is already exist, we don't change the request, just load the old one. I'll think. how to let user know, why is that so. Also, I will rename “Cancel” -> "Close"
|
|
Thank you @SofiaSazonova for all of your work iterating on this PR Overall - the process of opening up a new share request is much approved with these changes and will become a more streamlined process. An Important Remark(!):
Please take a look at the latest and if no additional concerns I will work with Sofia to get this PR approved and merged in the next day or two |
Thanks @SofiaSazonova for all the work on this PR! |
@anushka-singh here you go! |
Originally, the Request Purpose box used to be on the "Request Access" page. I see now it has moved to Share status page. Is there a reason for that? I think it made sense to have it on the "Request Access" Page along with the other fields that needed to be filled out. |
Before we had the Request Purpose box on both the Request Access initial page (opened from Catalog) and the Share Edit page (opened as second page from Catalog or from IMO - It felt less intuitive to have the edit on both pages and I thought it would be best to keep the text box in on Also the Share Object gets created on the first "Request Access" page (opened from Catalog) but if the share object already exists it just gets redirected to re-use the existing share object. Currently the logic we have in backend would just return the existing share object as is and not update the request purpose if one provided as well. Instead of adding more one-off logic to backend and additional conditions, it seemed best to clean up frontend views in this case Let me know if I missed anything in the above rationale @SofiaSazonova ++ If we can merge latest from OS main to resolve the issue with |
@noah-paige done) |
@noah-paige |
It's not quite right. The edit page (screenshot 3) appears immediately after the share created. So, just consider Request Access as step 1 and then Share fill in/edit form as step 2. No additional actions needed |
Okay! That makes sense. |
Hi @SofiaSazonova , The changes look really good. I was able to pull in your changes and checked the UI. The UI experience is really nice and I like that the user can create a share request on the same request modal itself. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
approving and will merge - thanks again @SofiaSazonova
…art 6 (Split APIs and graphql types) (#1320) ### Feature or Bugfix - Feature - Refactoring ### Detail As explained in the design for #1123 and #1283 we are trying to implement generic `datasets_base` and `shares_base` modules that can be used by any type of datasets and by any type of shareable object in a generic way. - First, this PR splits the query used in Worksheet and Environments to list glue databases and list datasets. They are pretty different queries, the one used in Worksheets is only relevant for S3 datasets, while the one in Environment is focused on the share items in general: - Introduce new API `listS3DatasetsSharedWithEnvGroup` to list shared glue databases in Worksheets view. It is part of the s3_datasets_shares module. This new API replaces the usage of `searchEnvironmentDataItems` in Worksheets frontend. - Remove Glue-parameters from `searchEnvironmentDataItems` API, this API belongs to shares_base. It is only used in the Environment view > Datasets tab, so I moved the API in frontend to modules/Environment. - remove unused parameters (`tables`, `locations`) from statistics in `api/types/ShareObjectStatistics`. Now the statistics are only generic. - Introduce new API `getS3ConsumptionData` in s3_datasets_shares. This new API call gets the details of gluedatabase/table, s3accesspoint that were previously part of ShareObject. This way the graphql ShareObject does not contain specific S3 info. - The rest of the APIs have been split in `shares_base` and `s3_datasets_shares`. In general, all the share lifecycle (create, add items, approve...) is part of shares_base. listDatasetShareObjects, verifyDatasetShareObjects used in the S3-Dataset UI are part of s3_datasets_shares - TODO: Review tests and create new tests for get_consumption_data. Currently tests for shares and datasets are placed in the same folder. I will open a separate PR to order the tests a bit before this ### Relates - #1283 - #1123 - #955 - #1277 ---> This PR needs to be merged and then I will introduce some changes in the ShareView. ### Security Please answer the questions below briefly where applicable, or write `N/A`. Based on [OWASP 10](https://owasp.org/Top10/en/). - Does this PR introduce or modify any input fields or queries - this includes fetching data from storage outside the application (e.g. a database, an S3 bucket)? - Is the input sanitized? - What precautions are you taking before deserializing the data you consume? - Is injection prevented by parametrizing queries? - Have you ensured no `eval` or similar functions are used? - Does this PR introduce any functionality or component that requires authorization? - How have you ensured it respects the existing AuthN/AuthZ mechanisms? - Are you logging failed auth attempts? - Are you using or adding any cryptographic features? - Do you use a standard proven implementations? - Are the used keys controlled by the customer? Where are they stored? - Are you introducing any new policies/roles/users? - Have you used the least-privilege principle? How? By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Feature or Bugfix
Detail
Relates
Security
Please answer the questions below briefly where applicable, or write
N/A
. Based onOWASP 10.
fetching data from storage outside the application (e.g. a database, an S3 bucket)?
eval
or similar functions are used?By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.