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

fix(owner): last modified on adding owner #9553

Merged
merged 6 commits into from
Jan 9, 2024

Conversation

anshbansal
Copy link
Collaborator

@anshbansal anshbansal commented Jan 3, 2024

  • Fix last modified not being updated on adding owner via the UI
  • Some cleanup in OwnerUtils to avoid copy-paste
  • Rename some confusing variable names
  • Optional should not be an argument to methods
  • Extract code out to make code readable in OwnerUtils

Things to pay special attention to

  • With the ownership type how do we define equality of an Owner? If owner urn is same but ownership type is different then is owner obj considered equal? There were 2 different definitions of equality in add owner and remove owner in OwnerUtils here. I have updated it to be what I consider to be correct equality definition. But please pay attention to that.

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

Copy link
Collaborator

@chriscollins3456 chriscollins3456 left a comment

Choose a reason for hiding this comment

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

looking solid! left one question about whether some logic is correct and then a suggestion for some tests

@anshbansal anshbansal force-pushed the ab-fix-lastmodified-update branch from ac8d1f6 to 7457c27 Compare January 9, 2024 13:07
assertTrue(OwnerUtils.isOwnerEqual(ownerWithTechnicalOwnership, ownerUrn1, technicalOwnershipTypeUrn));
assertFalse(OwnerUtils.isOwnerEqual(ownerWithBusinessOwnership, ownerUrn1, technicalOwnershipTypeUrn));
assertFalse(OwnerUtils.isOwnerEqual(ownerWithTechnicalOwnership, ownerUrn1, null));
assertTrue(OwnerUtils.isOwnerEqual(ownerWithoutOwnershipType, ownerUrn1, null));
Copy link
Collaborator

Choose a reason for hiding this comment

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

We are missing 1 test here I think:

assertTrue(OwnerUtils.isOwnerEqual(ownerWithoutOwnershipType, ownerUrn1, technicalOwnershipTypeUrn));

An owner without ownership type should be equal to the same owner being assigned a concrete ownership type so that all owners have a type of ownership assigned.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually I don't think this case is possible since the deprecated type is mandatory in our pdl definition.

However we can have:

Owner ownerWithoutOwnershipType = new Owner();
ownerWithoutOwnershipType.setOwner(ownerUrn1);
ownerWithoutOwnershipType.setType(NONE);

assertTrue(OwnerUtils.isOwnerEqual(ownerWithoutOwnershipType, ownerUrn1, technicalOwnershipTypeUrn));

Copy link
Collaborator

@pedro93 pedro93 left a comment

Choose a reason for hiding this comment

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

LGTM, wait for green CI.

@anshbansal anshbansal merged commit 5bfd674 into master Jan 9, 2024
34 checks passed
@anshbansal anshbansal deleted the ab-fix-lastmodified-update branch January 9, 2024 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product PR or Issue related to the DataHub UI/UX
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants