-
-
Notifications
You must be signed in to change notification settings - Fork 512
Exclude templates from isValid and isApproved publication checks #9422
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
67345fd
49a8fad
e6e0da3
b15d7f5
bcf95f2
41775a2
ce2c977
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1352,18 +1352,28 @@ private void updateOwnership(Integer groupIdentifier, | |
| } | ||
|
|
||
| /** | ||
| * For privileges to {@link ReservedGroup#all} group, check if it's allowed or not to publish invalid metadata. | ||
| * For privileges to {@link ReservedGroup#all} group, check if it's allowed or not to publish invalid or | ||
| * non-approved metadata. Templates (including sub-templates and templates of sub-templates) are exempt from | ||
| * these checks, as they are typically incomplete and not expected to be valid or approved. | ||
| * | ||
| * @param context | ||
| * @param messages | ||
| * @param metadata | ||
| * @param allowPublishInvalidMd | ||
| * @param allowPublishNonApprovedMd | ||
| * @throws Exception | ||
| * @param context the current service context | ||
| * @param messages the resource bundle for error messages | ||
| * @param metadata the metadata record being published | ||
| * @param allowPublishInvalidMd whether publishing invalid metadata is allowed (excluding templates) | ||
| * @param allowPublishNonApprovedMd whether publishing non-approved metadata is allowed (excluding templates) | ||
| * @throws Exception if the metadata is invalid or not approved and publishing is not allowed | ||
| */ | ||
| private void checkCanPublishToAllGroup(ServiceContext context, ResourceBundle messages, AbstractMetadata metadata, | ||
| boolean allowPublishInvalidMd, boolean allowPublishNonApprovedMd) throws Exception { | ||
|
|
||
| // Templates are typically incomplete and should not be subject to validation or approval checks | ||
| MetadataType metadataType = metadata.getDataInfo().getType(); | ||
| if (metadataType == MetadataType.TEMPLATE | ||
| || metadataType == MetadataType.SUB_TEMPLATE | ||
| || metadataType == MetadataType.TEMPLATE_OF_SUB_TEMPLATE) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is starting to be a little silly, perhaps MetadataType should have a .isRequiresValidation() property :)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in latest push |
||
| return; | ||
| } | ||
|
|
||
| if (!allowPublishInvalidMd) { | ||
| boolean hasValidation = | ||
| (metadataValidationRepository.count(MetadataValidationSpecs.hasMetadataId(metadata.getId())) > 0); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,7 +31,12 @@ | |
| import org.fao.geonet.api.records.model.SharingResponse; | ||
| import org.fao.geonet.domain.Group; | ||
| import org.fao.geonet.domain.GroupType; | ||
| import org.fao.geonet.domain.ISODate; | ||
| import org.fao.geonet.domain.Metadata; | ||
| import org.fao.geonet.domain.MetadataType; | ||
| import org.fao.geonet.domain.MetadataValidation; | ||
| import org.fao.geonet.domain.MetadataValidationId; | ||
| import org.fao.geonet.domain.MetadataValidationStatus; | ||
| import org.fao.geonet.domain.OperationAllowed; | ||
| import org.fao.geonet.domain.Profile; | ||
| import org.fao.geonet.domain.ReservedGroup; | ||
|
|
@@ -44,7 +49,9 @@ | |
| import org.fao.geonet.kernel.setting.Settings; | ||
| import org.fao.geonet.repository.MetadataRepository; | ||
| import org.fao.geonet.repository.MetadataStatusRepository; | ||
| import org.fao.geonet.repository.MetadataValidationRepository; | ||
| import org.fao.geonet.repository.OperationAllowedRepository; | ||
| import org.fao.geonet.repository.StatusValueRepository; | ||
| import org.fao.geonet.repository.UserRepositoryTest; | ||
| import org.fao.geonet.services.AbstractServiceIntegrationTest; | ||
| import org.junit.Before; | ||
|
|
@@ -87,6 +94,12 @@ public class MetadataSharingApiTest extends AbstractServiceIntegrationTest { | |
| @Autowired | ||
| private MetadataStatusRepository metadataStatusRepository; | ||
|
|
||
| @Autowired | ||
| private MetadataValidationRepository metadataValidationRepository; | ||
|
|
||
| @Autowired | ||
| private StatusValueRepository statusValueRepository; | ||
|
|
||
| @Autowired | ||
| private SettingManager settingManager; | ||
|
|
||
|
|
@@ -643,4 +656,82 @@ private void grantUserAdminInNewGroup(User user, String groupName) { | |
| .setProfile(Profile.UserAdmin) | ||
| .setUser(user)); | ||
| } | ||
|
|
||
| /** | ||
| * Verifies that publishing a TEMPLATE succeeds even when the metadata has a required invalid | ||
| * validation record and {@code allowPublishInvalidMd} is {@code false}. | ||
| * Templates are exempt from validation checks. | ||
| */ | ||
| @Test | ||
| public void publishInvalidTemplateSucceedsWhenPublishInvalidMdDisabled() throws Exception { | ||
| // Disable publishing of invalid metadata – the strict mode that triggered the bug. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After the bug is addressed this comment will not make sense :P
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in latest push |
||
| settingManager.setValue(Settings.METADATA_WORKFLOW_ALLOW_PUBLISH_INVALID_MD, false); | ||
|
|
||
| // Change the metadata type to TEMPLATE. | ||
| Metadata template = metadataRepository.findById(metadataId).get(); | ||
| template.getDataInfo().setType(MetadataType.TEMPLATE); | ||
| metadataRepository.save(template); | ||
|
|
||
| // Persist a required, INVALID validation record so the check would fail for normal metadata. | ||
| MetadataValidation invalidValidation = new MetadataValidation() | ||
| .setId(new MetadataValidationId(metadataId, "xsd")) | ||
| .setStatus(MetadataValidationStatus.INVALID) | ||
| .setRequired(true) | ||
| .setNumTests(1) | ||
| .setNumFailures(1); | ||
| metadataValidationRepository.save(invalidValidation); | ||
|
|
||
| MockMvc mockMvc = MockMvcBuilders.webAppContextSetup(this.wac).build(); | ||
| MockHttpSession mockHttpSession = loginAs(reviewerUser); | ||
|
|
||
| // Publishing the template must succeed (HTTP 204) despite the invalid validation record. | ||
| mockMvc.perform(put("/srv/api/records/" + metadataUuid + "/publish") | ||
| .session(mockHttpSession)) | ||
| .andExpect(status().isNoContent()); | ||
|
|
||
| // Confirm publication privileges were actually granted. | ||
| List<OperationAllowed> ops = operationAllowedRepository.findAllById_MetadataId(metadataId); | ||
| boolean published = ops.stream().anyMatch(op -> ReservedGroup.isReserved(op.getId().getGroupId())); | ||
| assertTrue("Template should be published even when it has an invalid validation record", published); | ||
| } | ||
|
|
||
| /** | ||
| * Verifies that publishing a TEMPLATE succeeds even when its workflow status is DRAFT | ||
| * (non-approved) and {@code allowPublishNonApprovedMd} is {@code false}. | ||
| * Templates are exempt from approval status checks. | ||
| */ | ||
| @Test | ||
| public void publishNonApprovedTemplateSucceedsWhenPublishNonApprovedMdDisabled() throws Exception { | ||
| // Disable publishing of non-approved metadata – the strict mode that triggered the bug. | ||
| settingManager.setValue(Settings.METADATA_WORKFLOW_ALLOW_PUBLISH_NON_APPROVED_MD, false); | ||
|
|
||
| // Change the metadata type to TEMPLATE. | ||
| Metadata template = metadataRepository.findById(metadataId).get(); | ||
| template.getDataInfo().setType(MetadataType.TEMPLATE); | ||
| metadataRepository.save(template); | ||
|
|
||
| // Assign a DRAFT (non-approved) workflow status so the check would fail for normal metadata. | ||
| MetadataStatus draftStatus = new MetadataStatus(); | ||
| draftStatus.setMetadataId(metadataId); | ||
| draftStatus.setChangeDate(new ISODate()); | ||
| draftStatus.setUserId(1); | ||
| draftStatus.setChangeMessage("draft status for template publish test"); | ||
| draftStatus.setOwner(1); | ||
| draftStatus.setUuid(metadataUuid); | ||
| draftStatus.setStatusValue(statusValueRepository.findById(Integer.parseInt(StatusValue.Status.DRAFT)).get()); | ||
| metadataStatusRepository.save(draftStatus); | ||
|
|
||
| MockMvc mockMvc = MockMvcBuilders.webAppContextSetup(this.wac).build(); | ||
| MockHttpSession mockHttpSession = loginAs(reviewerUser); | ||
|
|
||
| // Publishing the template must succeed (HTTP 204) despite the non-approved status. | ||
| mockMvc.perform(put("/srv/api/records/" + metadataUuid + "/publish") | ||
| .session(mockHttpSession)) | ||
| .andExpect(status().isNoContent()); | ||
|
|
||
| // Confirm publication privileges were actually granted. | ||
| List<OperationAllowed> ops = operationAllowedRepository.findAllById_MetadataId(metadataId); | ||
| boolean published = ops.stream().anyMatch(op -> ReservedGroup.isReserved(op.getId().getGroupId())); | ||
| assertTrue("Template should be published even when its workflow status is not approved", published); | ||
| } | ||
| } | ||
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.
It is unclear to me if tempaltes may be subject to an approval workflow? I could very much imagine a team wishing to approve a new workflow before sharing it for wider use?
Uh oh!
There was an error while loading. Please reload this page.
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.
If it is agreed that templates should respect the Allow publication of non-approved metadata setting, maybe the Allow submission/approval of invalid metadata setting needs to also exclude templates instead. With that setting disabled, currently an invalid template could never be approved so again it would be unpublishable.
Updated the PR description accordingly
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.
Updated the code so the "must be approved to publish" check is still respected. Instead I modified the "must be valid to approve" check to exclude templates.
So now an invalid template can be approved and then published.