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

OWLAP-51: Effectivetime restore updates #338

Merged
merged 18 commits into from
Apr 4, 2019

Conversation

zoliMolnar
Copy link

This pull request adds functionality to restore effective time on members and components.

@zoliMolnar zoliMolnar self-assigned this Apr 1, 2019
@zoliMolnar zoliMolnar requested review from cmark and nagyo April 1, 2019 16:24
&& memberToRestore.getCharacteristicTypeId().equals(previousMember.getProperties().get(SnomedRf2Headers.FIELD_CHARACTERISTIC_TYPE_ID))
&& memberToRestore.getTypeId().equals(previousMember.getProperties().get(SnomedRf2Headers.FIELD_TYPE_ID))
&& memberToRestore.getGroup() == ((Integer )previousMember.getProperties().get(SnomedRf2Headers.FIELD_RELATIONSHIP_GROUP)).intValue();
} else if(componentToRestore instanceof SnomedDescriptionTypeRefSetMember) {
Copy link
Member

Choose a reason for hiding this comment

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

Fix spacing, please! :)

Copy link
Author

Choose a reason for hiding this comment

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

This doesn't look the same inside my IDE.

final String previousRuleStrengthId = (String) previousMember.getProperties().get(SnomedRf2Headers.FIELD_MRCM_RULE_STRENGTH_ID);
final String previousContentTypeId = (String) previousMember.getProperties().get(SnomedRf2Headers.FIELD_MRCM_CONTENT_TYPE_ID);

if (previousdGrouped != null && previousdGrouped.booleanValue() != memberToRestore.isGrouped()) {
Copy link
Member

Choose a reason for hiding this comment

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

Typo!

return false;
}

if (previousAtributeInGroupCardinality != null && !previousAtributeInGroupCardinality.equals(memberToRestore.getAttributeInGroupCardinality())) {
Copy link
Member

Choose a reason for hiding this comment

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

Typo!

final String previousDomainTemplateForPostcoordination = (String) previousMember.getProperties().get(SnomedRf2Headers.FIELD_MRCM_DOMAIN_TEMPLATE_FOR_POSTCOORDINATION);
final String previousEditorialGuideReference = (String) previousMember.getProperties().get(SnomedRf2Headers.FIELD_MRCM_EDITORIAL_GUIDE_REFERENCE);

if (previousDomainConstraint != null && !previousDomainConstraint.equals(memberToRestore.getDomainConstraint())) {
Copy link
Member

Choose a reason for hiding this comment

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

Use java.util.Objects.equals() to eliminate unnecessary null checks.

}

// XXX: Should never happen
throw new IllegalStateException();
Copy link
Member

Choose a reason for hiding this comment

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

In this case, throw an UnsupportedOperationException or IllegalArgumentException with a proper message.

if (componentsByType.isEmpty()) return;

final List<String> branchesForPreviousVersion = getAvailableVersionPaths();
for (String branch : branchesForPreviousVersion) {
Copy link
Member

Choose a reason for hiding this comment

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

This list of branches provided here has the following restrictions:

  • the previous version of the component must exist on at least one branch. if we can't find it on any of the branches then we should propagate that upwards. Thoughts @cmark?
  • it is not guaranteed that the component exists on all branches
  • if we find the component on a branch then we must not look further on the next available branch

Copy link
Member

Choose a reason for hiding this comment

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

I might even change the two for loops here. First iterate over types and then branches?

return memberToRestore.isActive() == previousMember.isActive() && memberToRestore.getModuleId().equals(previousMember.getModuleId());
} else if (componentToRestore instanceof SnomedMRCMAttributeDomainRefSetMember) {
final SnomedMRCMAttributeDomainRefSetMember memberToRestore = (SnomedMRCMAttributeDomainRefSetMember) componentToRestore;
final Boolean previousdGrouped = ClassUtils.checkAndCast(previousMember.getProperties().get(SnomedRf2Headers.FIELD_MRCM_GROUPED), Boolean.class);
Copy link
Member

Choose a reason for hiding this comment

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

Typo!

final SnomedMRCMAttributeDomainRefSetMember memberToRestore = (SnomedMRCMAttributeDomainRefSetMember) componentToRestore;
final Boolean previousdGrouped = ClassUtils.checkAndCast(previousMember.getProperties().get(SnomedRf2Headers.FIELD_MRCM_GROUPED), Boolean.class);
final String previousAttributeCardinality = (String) previousMember.getProperties().get(SnomedRf2Headers.FIELD_MRCM_ATTRIBUTE_CARDINALITY);
final String previousAtributeInGroupCardinality = (String) previousMember.getProperties().get(SnomedRf2Headers.FIELD_MRCM_ATTRIBUTE_IN_GROUP_CARDINALITY);
Copy link
Member

Choose a reason for hiding this comment

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

Typo!

}
} else {
throw new UnexpectedTypeException("Unexpected member type '" + componentToRestore.getClass() + "'.");
}
Copy link
Member

Choose a reason for hiding this comment

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

That's quite a long method but we are on a time schedule now, so I guess it should be ok.

@@ -791,8 +791,11 @@ public void preCommit() {
}
}
}

new EffectiveTimeRestorer(getBranch()).restoreEffectiveTimes(transaction.getChangeSetData().getChangedObjects().stream()
Copy link
Member

Choose a reason for hiding this comment

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

Why not instantiate the restorer in the constructor and just pass the branch to the method?

Copy link
Member

@cmark cmark left a comment

Choose a reason for hiding this comment

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

LGTM!

@cmark cmark merged commit 1344e68 into 6.x Apr 4, 2019
@cmark cmark deleted the feature/OWLAP-51_effectivetime_restore_updates branch April 4, 2019 11:26
@cmark cmark restored the feature/OWLAP-51_effectivetime_restore_updates branch April 4, 2019 11:26
@cmark cmark deleted the feature/OWLAP-51_effectivetime_restore_updates branch July 18, 2019 07:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants