feat(ui): Changes to allow editable display name for datasets and editable business label for schema fields #6171

jaykadambi · 2022-10-11T22:36:14Z

Summary: In reference to the discussion in Slack with @shirshanka and @gabe-lyons, creating this PR to allow for editable searchable name (aka display name) for datasets and to allow for presenting and being able to edit label (aka business label) at the schema field level. Please see example screenshots below.

Examples:

Checklist

The PR conforms to DataHub's Contributing Guideline (particularly Commit Message Format)
Links to related issues (if applicable)
Tests for the changes have been added/updated (if applicable)
Docs related to the changes have been added/updated (if applicable). If a new feature has been added a Usage Guide has been added for the same.
For any breaking change/potential downtime/deprecation/big changes an entry has been made in Updating DataHub

…ness label for schema fields

github-actions · 2022-10-15T02:34:36Z

Unit Test Results (metadata ingestion)

    6 files -       2   6 suites - 2 50m 36s ⏱️ - 7m 58s
  52 tests -   679 46 ✔️ -   683 3 💤 +1 0 ❌ ±0 3 🔥 +3
106 runs - 1 358 97 ✔️ - 1 363 6 💤 +2 0 ❌ ±0 3 🔥 +3

For more details on these errors, see this check.

Results for commit 33b8432. ± Comparison against base commit 95d1e01.

This pull request removes 679 tests.

tests.integration.azure_ad.test_azure_ad ‑ test_azure_ad_config
tests.integration.azure_ad.test_azure_ad ‑ test_azure_ad_source_default_configs
tests.integration.azure_ad.test_azure_ad ‑ test_azure_ad_source_nested_groups
tests.integration.azure_ad.test_azure_ad ‑ test_azure_source_ingestion_disabled
tests.integration.bigquery-usage.test_bigquery_usage ‑ test_bq_timezone_validation
tests.integration.bigquery-usage.test_bigquery_usage ‑ test_bq_usage_config
tests.integration.bigquery-usage.test_bigquery_usage ‑ test_bq_usage_source
tests.integration.bigquery-usage.test_bigquery_usage ‑ test_bq_usage_source_with_read_events
tests.integration.bigquery-usage.test_bigquery_usage ‑ test_remove_extras[20220101-test_dataset]
tests.integration.bigquery-usage.test_bigquery_usage ‑ test_remove_extras[test_table$20220101-test_table]
…

This pull request skips 1 test.

tests.integration.trino.test_trino ‑ test_trino_ingest

♻️ This comment has been updated with latest results.

github-actions · 2022-10-15T02:41:01Z

Unit Test Results (build & test)

597 tests ±0 593 ✔️ ±0 11m 42s ⏱️ -17s
147 suites ±0     4 💤 ±0
147 files ±0     0 ❌ ±0

Results for commit 33b8432. ± Comparison against base commit 95d1e01.

♻️ This comment has been updated with latest results.

jjoyce0510 · 2022-11-15T00:37:28Z

datahub-web-react/src/app/entity/shared/tabs/Documentation/components/DescriptionEditor.tsx

-            variables: { urn: mutationUrn, input: { editableProperties: { description: sanitizedDescription || '' } } },
+            variables: {
+                urn: mutationUrn,
+                input: { editableProperties: { name: editableName, description: sanitizedDescription || '' } },


This will actually update the name to be an empty string (" ") if one was not previously set. This is not what we want .. Instead, we should be able to leave editable name null if there is not one set

This makes sense to me and I will make the update.

jjoyce0510 · 2022-11-15T00:40:28Z

datahub-web-react/src/app/entity/shared/containers/profile/header/EntityName.tsx

@@ -37,10 +39,32 @@ function EntityName(props: Props) {

    const [updateName] = useUpdateNameMutation();

+    const updateEntity = useEntityUpdate<GenericEntityUpdate>();
+
+    const updateEditableName = (entityNameUpdate, mutationUrn) => {


Minor comment here: Name is a very important concept. It's important that we keep the APIs standard across all entity types when manipulating this field. I would strongly request that we use the more structured "updateName" mutation to handle updates to any entity's name. This is similar to what we do with the updateDescription mutation, which abstracts away description updates for each type of entity

What do you think?

The reason this was built this way is to support being able to allow updating the editableDatasetProperties' Name field (and keeping the non-editable Name field intact). The current updateName mutation does not support Dataset entity name updates as I can understand (/datahub/graphql/resolvers/mutate/UpdateNameResolver.java).

If this is agreeable, I can extend the existing UpdateNameResolver to support Dataset entity's EditableDatasetProperties' Name field. Please advise and I can update it accordingly and abstract it from the UI perspective by just calling "updateName" mutation.

I have made this change with the latest commit. Please look into.

jjoyce0510 · 2022-11-15T00:41:58Z

datahub-web-react/src/app/entity/shared/containers/profile/header/EntityHeader.tsx

    switch (entityType) {
+        case EntityType.Dataset: {


I definitely think that we should avoid hardcoding in whether the current user is able to edit the name..

This requires a way to ask our backend whether the current user is able to change the name. Likely we'll need a new Metadata Privilege called "Update Entity Name" that will be able to represent this. Then we'll need to fetch those privileges from the frontend

This is a specific need that we have on our end wherein only the owners must be able to edit certain fields within the UI. I have added this as a new privilege similar to manageGlossaries and fetch and matched it in the frontend.

Hey @jaykadambi are you still working on this PR?

@aditya-radhakrishnan - Yes. I had some questions/comments back to @jjoyce0510. Perhaps I can go ahead and commit the updated code and let @jjoyce0510 review when he gets a chance. Thanks!

@jjoyce0510 - I have made the next set of commits to address the PR comments. Please review and let me know your feedback. Thanks!

cc - @aditya-radhakrishnan.

jjoyce0510 · 2022-11-15T00:42:30Z

datahub-web-react/src/app/entity/dataset/profile/schema/components/SchemaLabelField.tsx

+
+const ABBREVIATED_LIMIT = 40;
+
+export default function LabelField({ label, onUpdate, isEdited = false, original }: Props) {


jjoyce0510 · 2022-11-15T00:43:05Z

datahub-graphql-core/src/main/resources/entity.graphql

+    """
+    An optional sub resource type
+    """
+    subResourceType: SubResourceType


These should have ! to be required, since this only applies to DatasetFieldLabels

Overall it's looking in the right direction! I have a couple actionable questions + pieces of feedback. Let me know what you think!

@jjoyce0510 - Thanks for reviewing the PR and providing your feedback. I have couple of items to your feedback. Please check and let me know your thoughts on those and I will work on updating the PR with the updated code asap. Thanks!

jjoyce0510 · 2022-11-15T00:43:34Z

datahub-graphql-core/src/main/resources/entity.graphql

@@ -7106,6 +7131,31 @@ input DescriptionUpdateInput {
    subResource: String
 }

+"""
+Updates the label of a resource. Currently supports DatasetField labels only
+"""


Label is maybe a bit too generic of a name here. This is strictly restricted to Dataset Fields, and should be named as such.

Consider DatasetFieldLabelUpdateInput

Sounds good

jjoyce0510 · 2022-11-15T00:43:44Z

datahub-graphql-core/src/main/resources/entity.graphql

+    """
+    Updates the label of a resource. Currently only supports Dataset Schema Fields
+    """
+    updateLabel(input: LabelUpdateInput!): Boolean


updateDatasetFieldLabel

jjoyce0510

Overall it's looking in the right direction! I have a couple actionable questions + pieces of feedback. Let me know what you think!

aditya-radhakrishnan · 2022-12-07T19:48:03Z

Hey @jaykadambi are you still working on this PR?

jaykadambi · 2022-12-07T23:36:03Z

Hey @jaykadambi are you still working on this PR?

@aditya-radhakrishnan - Yes. I had some questions/comments back to @jjoyce0510. Perhaps I can go ahead and commit the updated code and let @jjoyce0510 review when he gets a chance. Thanks!

jaykadambi · 2023-01-04T17:45:15Z

@aditya-radhakrishnan @jjoyce0510 - please advise on the updated changes in this PR that I had made a few days ago. Thanks!

jjoyce0510

Hi @jaykadambi ! Thank you for the amazing contribution.

I've had time to sync with my team on this contribution. They raised concerns about the addition of a new concept of "field label" into the main repository.

Folks were generally receptive above allowing users to update the name for an entity, but label was a point of contention, since it's a new concept that we'll need to support even though we have not received significant feedback to indicate that it's a widespread problem.

If you're willing, we'd like to propose an alternative: Split this PR into an initial PR which isolates the changes for updating the "name" of an asset. Clearly delineate which entities we support updating name for, and which we do not. Separately, we can discuss whether the field name label is a concept DataHub should natively support separately. One possibility is that DataHub is not currently ready to accept this contribution.

We really appreciate the tremendous amount of work you've already put into this PR, and apologies that I personally did not raise this issue earlier.

Thanks for being a great DataHub community member! Looking forward to hearing more.

Cheers
John

jaykadambi · 2023-01-18T23:07:30Z

Hi @jaykadambi ! Thank you for the amazing contribution.

I've had time to sync with my team on this contribution. They raised concerns about the addition of a new concept of "field label" into the main repository.

Folks were generally receptive above allowing users to update the name for an entity, but label was a point of contention, since it's a new concept that we'll need to support even though we have not received significant feedback to indicate that it's a widespread problem.

If you're willing, we'd like to propose an alternative: Split this PR into an initial PR which isolates the changes for updating the "name" of an asset. Clearly delineate which entities we support updating name for, and which we do not. Separately, we can discuss whether the field name label is a concept DataHub should natively support separately. One possibility is that DataHub is not currently ready to accept this contribution.

We really appreciate the tremendous amount of work you've already put into this PR, and apologies that I personally did not raise this issue earlier.

Thanks for being a great DataHub community member! Looking forward to hearing more.

Cheers John

Thanks @jjoyce0510 for the detailed note. I will submit a new PR as you have suggested. Thanks!

jjoyce0510 · 2023-01-24T03:16:17Z

Going to close this PR for now then, will be eagerly awaiting to hear more from you!

Thanks
John

Changes to allow editable display name for datasets and editable busi…

a163980

…ness label for schema fields

github-actions bot added the product PR or Issue related to the DataHub UI/UX label Oct 11, 2022

maggiehays added the community-contribution PR or Issue raised by member(s) of DataHub Community label Oct 13, 2022

Fix order of parameters

33b8432

gabe-lyons self-requested a review October 18, 2022 15:03

maggiehays added the hacktoberfest-accepted Acceptance for hacktoberfest https://hacktoberfest.com/participation/ label Oct 21, 2022

jjoyce0510 reviewed Nov 15, 2022

View reviewed changes

Address PR review comments

7d5a200

aditya-radhakrishnan self-requested a review December 19, 2022 20:07

jjoyce0510 reviewed Jan 10, 2023

View reviewed changes

aditya-radhakrishnan removed their request for review January 10, 2023 04:26

jjoyce0510 closed this Jan 24, 2023

jayasimhankv mentioned this pull request May 28, 2024

Changes to allow editable dataset name #10608

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ui): Changes to allow editable display name for datasets and editable business label for schema fields #6171

feat(ui): Changes to allow editable display name for datasets and editable business label for schema fields #6171

jaykadambi commented Oct 11, 2022

github-actions bot commented Oct 15, 2022 •

edited

Loading

github-actions bot commented Oct 15, 2022 •

edited

Loading

jjoyce0510 Nov 15, 2022

jaykadambi Nov 16, 2022

jjoyce0510 Nov 15, 2022

jaykadambi Nov 16, 2022

jaykadambi Dec 9, 2022

jjoyce0510 Nov 15, 2022

jaykadambi Nov 16, 2022 •

edited

Loading

jaykadambi Dec 9, 2022 •

edited

Loading

jjoyce0510 Nov 15, 2022

jjoyce0510 Nov 15, 2022

jaykadambi Nov 17, 2022

jaykadambi Dec 9, 2022

jjoyce0510 Nov 15, 2022

jaykadambi Nov 17, 2022

jaykadambi Dec 9, 2022

jjoyce0510 Nov 15, 2022

jaykadambi Nov 17, 2022

jjoyce0510 left a comment

aditya-radhakrishnan commented Dec 7, 2022

jaykadambi commented Dec 7, 2022

jaykadambi commented Jan 4, 2023

jjoyce0510 left a comment

jaykadambi commented Jan 18, 2023

jjoyce0510 commented Jan 24, 2023


		const ABBREVIATED_LIMIT = 40;

		export default function LabelField({ label, onUpdate, isEdited = false, original }: Props) {

feat(ui): Changes to allow editable display name for datasets and editable business label for schema fields #6171

feat(ui): Changes to allow editable display name for datasets and editable business label for schema fields #6171

Conversation

jaykadambi commented Oct 11, 2022

Checklist

github-actions bot commented Oct 15, 2022 • edited Loading

Unit Test Results (metadata ingestion)

github-actions bot commented Oct 15, 2022 • edited Loading

Unit Test Results (build & test)

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

jaykadambi Nov 16, 2022 • edited Loading

Choose a reason for hiding this comment

jaykadambi Dec 9, 2022 • 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

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

jjoyce0510 left a comment

Choose a reason for hiding this comment

aditya-radhakrishnan commented Dec 7, 2022

jaykadambi commented Dec 7, 2022

jaykadambi commented Jan 4, 2023

jjoyce0510 left a comment

Choose a reason for hiding this comment

jaykadambi commented Jan 18, 2023

jjoyce0510 commented Jan 24, 2023

github-actions bot commented Oct 15, 2022 •

edited

Loading

github-actions bot commented Oct 15, 2022 •

edited

Loading

jaykadambi Nov 16, 2022 •

edited

Loading

jaykadambi Dec 9, 2022 •

edited

Loading