Worked on the feature to update group title #6047

Ankit-Keshari-Vituity · 2022-09-26T05:51:35Z

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

github-actions · 2022-09-26T06:13:07Z

Unit Test Results (build & test)

591 tests 587 ✔️ 13m 42s ⏱️
146 suites     4 💤
146 files     0 ❌

Results for commit 900dba3.

♻️ This comment has been updated with latest results.

chriscollins3456

Nice! some small comments then i think this is good to go

chriscollins3456 · 2022-09-30T13:51:56Z

...l-core/src/main/java/com/linkedin/datahub/graphql/resolvers/domain/CreateDomainResolver.java

+        String tagUrn = _entityClient.ingestProposal(proposal, context.getAuthentication());
+        OwnerUtils.addCreatorAsOwner(context, tagUrn, OwnerEntityType.CORP_USER, OwnershipType.TECHNICAL_OWNER, _entityService);


you can change tagUrn -> domainUrn

Checked and Fixed!!

chriscollins3456 · 2022-09-30T13:52:17Z

...rc/main/java/com/linkedin/datahub/graphql/resolvers/glossary/CreateGlossaryNodeResolver.java

+          String tagUrn = _entityClient.ingestProposal(proposal, context.getAuthentication());
+          OwnerUtils.addCreatorAsOwner(context, tagUrn, OwnerEntityType.CORP_USER, OwnershipType.TECHNICAL_OWNER, _entityService);


same, tagUrn -> glossaryNodeUrn

Checked and Fixed!!

chriscollins3456 · 2022-09-30T13:52:41Z

...rc/main/java/com/linkedin/datahub/graphql/resolvers/glossary/CreateGlossaryTermResolver.java

+          String tagUrn = _entityClient.ingestProposal(proposal, context.getAuthentication());
+          OwnerUtils.addCreatorAsOwner(context, tagUrn, OwnerEntityType.CORP_USER, OwnershipType.TECHNICAL_OWNER, _entityService);
+          return tagUrn;


tagUrn -> glossaryTermUrn

Checked and Fixed!!

chriscollins3456 · 2022-09-30T14:11:37Z

datahub-web-react/src/app/entity/group/GroupInfoSideBar.tsx

@@ -91,6 +103,29 @@ export default function GroupInfoSidebar({ sideBarData, refetch }: Props) {
    /* eslint-disable @typescript-eslint/no-unused-vars */
    const [editGroupModal, showEditGroupModal] = useState(false);
    const canEditGroup = true; // TODO; Replace this will fine-grained understanding of user permissions.


can you update this to reflect our permissions? you can do something like

const me = useGetAuthenticatedUser(); const canEditGroup = me?.platformPrivileges.manageIdentities;

and then can you add a check to ensure that they have these permissions before making the name editable?

Checked and Fixed!!

chriscollins3456

looking good! the comments I have are around the functionality change fro DeprecationPill.

In the future, when you have separate projects can you put them in separate PRs? for example, your work on editing group names and adding owners on creation of terms, nodes, and domains is done, but now it's intermingled with your new deprecation pill work.

chriscollins3456 · 2022-10-03T14:48:31Z

datahub-web-react/src/app/entity/group/GroupInfoSideBar.tsx

+                            <GroupTitle level={3} editable={{ onChange: handleTitleUpdate }}>
+                                {groupTitle}
+                            </GroupTitle>


can you use the new canEditGroup to make it so they can't edit the name if they can't edit the group? check below for the aboutText where we say editable={canEditGroup ? { onChange: onSaveAboutMe } : false} as an example

Checked and Fixed!!

Sure. For the next time onward I'll definitely create a new PR for each task.

chriscollins3456 · 2022-10-03T15:06:58Z

datahub-web-react/src/app/entity/shared/components/styled/DeprecationPill.tsx

                        )}
+                        {isDividerNeeded && <ThinDivider />}
+                        <Button
+                            type="default"


Instead of using Button here, can we actually just make this text with font-size 12px and a hover state with blue[5] as the color on hover?
Here's some screenshots of what I'm thinking:

on hover:

Checked and Fixed!!

chriscollins3456 · 2022-10-03T15:10:02Z

datahub-web-react/src/app/entity/shared/components/styled/DeprecationPill.tsx

@@ -56,11 +57,14 @@ const StyledInfoCircleOutlined = styled(InfoCircleOutlined)`
 `;

 type Props = {
+    urns: Array<string>;


while the mutation below is batch, I think this pill is only relevant to one entity at a time, so I think this can be a singular urn.

Checked and Fixed!!

chriscollins3456 · 2022-10-03T15:11:29Z

datahub-web-react/src/app/entity/shared/components/styled/DeprecationPill.tsx

+                                })
+                            }
+                        >
+                            Mark as un-deprecated


I also don't think we want this button to be shown every time we show this pill. ie. we don't want them to be able to undeprecate from the search results page so DefaultPrevieCard instances of this pill shouldn't show this button.

What do you think about adding a new optional prop called showUndeprecate that will show this button if you pass it in?

Checked and Fixed!!

chriscollins3456 · 2022-10-03T15:11:59Z

datahub-web-react/src/app/entity/shared/components/styled/DeprecationPill.tsx

-                                    <LastEvaluatedAtLabel>{decommissionTimeLocal}</LastEvaluatedAtLabel>
-                                </Tooltip>
-                            </Typography.Text>
+                            <>


I don't think you need these extra fragments around the decommission time piece

chriscollins3456

nice!

chriscollins3456 · 2022-10-04T14:53:45Z

you're getting failing tests on CreateDomainResolverTest, CreateGlossaryNodeResolverTest, and CreateGlossaryTermResolverTest. Anywhere you instantiate CreateDomainResolver, CreateGlossaryNodeResolver, or CreateGlossaryTermResolver in those tests, you're going to need to pass in a new mock entity service.

Check out AddTagsResolverTest.java where we pass in EntityService mockService = Mockito.mock(EntityService.class); to AddTagsResolver resolver = new AddTagsResolver(mockService);

chriscollins3456 · 2022-10-05T14:11:10Z

...est/java/com/linkedin/datahub/graphql/resolvers/glossary/CreateGlossaryTermResolverTest.java

@@ -10,7 +10,7 @@
 import com.linkedin.metadata.Constants;
 import com.linkedin.metadata.key.GlossaryTermKey;
 import com.linkedin.metadata.utils.GenericRecordUtils;
-import com.linkedin.mxe.MetadataChangeProposal;
+gitimport com.linkedin.mxe.MetadataChangeProposal;


gotta remove the "git" here as that's causing a failing build

Worked on the feature to update group tile

d8a0290

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

Worked on the backend part of update group name feature

3df0239

Ankit-Keshari-Vituity added 3 commits September 27, 2022 13:01

Typo error and funtion changed

c23b060

Worked on the create owner type

ba6495d

Worked on the issues after build

3fc1735

chriscollins3456 reviewed Sep 30, 2022

View reviewed changes

Ankit-Keshari-Vituity added 2 commits October 2, 2022 18:47

Worked on the PR comments mention on my PR

1528938

Worked on the un-deprecated feature in pop-over

f0b385a

chriscollins3456 requested changes Oct 3, 2022

View reviewed changes

Ankit-Keshari-Vituity added 4 commits October 3, 2022 21:34

Worked on the PR comments mentioned

df07ec6

Fixed Style issue

1f05dfe

Typo issue

4a10b22

Typo issue

eaf22b9

chriscollins3456 approved these changes Oct 3, 2022

View reviewed changes

Worked on the faild test case

ee46aae

Worked on the failed test cases

e41267d

chriscollins3456 reviewed Oct 5, 2022

View reviewed changes

Ankit-Keshari-Vituity added 2 commits October 6, 2022 10:58

Removed type

63ec730

Resolved the build issue

900dba3

chriscollins3456 merged commit 11092c7 into datahub-project:master Oct 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Worked on the feature to update group title #6047

Worked on the feature to update group title #6047

Ankit-Keshari-Vituity commented Sep 26, 2022

github-actions bot commented Sep 26, 2022 •

edited

Loading

chriscollins3456 left a comment

chriscollins3456 Sep 30, 2022

Ankit-Keshari-Vituity Oct 3, 2022

chriscollins3456 Sep 30, 2022

Ankit-Keshari-Vituity Oct 3, 2022

chriscollins3456 Sep 30, 2022

Ankit-Keshari-Vituity Oct 3, 2022

chriscollins3456 Sep 30, 2022

Ankit-Keshari-Vituity Oct 3, 2022

chriscollins3456 left a comment

chriscollins3456 Oct 3, 2022

Ankit-Keshari-Vituity Oct 3, 2022

chriscollins3456 Oct 3, 2022

Ankit-Keshari-Vituity Oct 3, 2022

chriscollins3456 Oct 3, 2022

Ankit-Keshari-Vituity Oct 3, 2022

chriscollins3456 Oct 3, 2022

Ankit-Keshari-Vituity Oct 3, 2022

chriscollins3456 Oct 3, 2022

chriscollins3456 left a comment

chriscollins3456 commented Oct 4, 2022

chriscollins3456 Oct 5, 2022

		String tagUrn = _entityClient.ingestProposal(proposal, context.getAuthentication());
		OwnerUtils.addCreatorAsOwner(context, tagUrn, OwnerEntityType.CORP_USER, OwnershipType.TECHNICAL_OWNER, _entityService);

Worked on the feature to update group title #6047

Worked on the feature to update group title #6047

Conversation

Ankit-Keshari-Vituity commented Sep 26, 2022

Checklist

github-actions bot commented Sep 26, 2022 • edited Loading

Unit Test Results (build & test)

chriscollins3456 left a 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

chriscollins3456 left a 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

Choose a reason for hiding this comment

chriscollins3456 left a comment

Choose a reason for hiding this comment

chriscollins3456 commented Oct 4, 2022

Choose a reason for hiding this comment

github-actions bot commented Sep 26, 2022 •

edited

Loading