Skip to content

Conversation

@yashmeet29
Copy link
Collaborator

@yashmeet29 yashmeet29 commented Oct 29, 2025

Describe your changes

In copyAttachments method after copying the attachments to target entity, we need to update the draft tables metadata with newly copied attachments in target entity. To update the filename, we were reading "cmis:contentStreamFileName" property of response, which doesn't changes if we are renaming the attachments. So, on UI always the initial name was showing. Updated "cmis:contentStreamFileName" with "cmis:name" which will be updated with every name update.

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Checklist before requesting a review

  • I follow Java Development Guidelines for SAP
  • I have tested the functionality on my cloud environment.
  • I have provided sufficient automated/ unit tests for the code.
  • I have increased or maintained the test coverage.
  • I have ran integration tests on my cloud environment.
  • I have validated blackduck portal for any vulnerability after my commit.

Upload Screenshots/lists of the scenarios tested

  • I have Uploaded Screenshots or added lists of the scenarios tested in description
    Create an entity and upload an attachment, Save. Edit entity, rename attachment, Save. Create another entity and Copy the attachment from 1st entity. Attachment copied with the updated name.

Create an entity and create a link, Save. Edit entity, rename link, Save. Create another entity and Copy the link from 1st entity. Link copied with the updated name.

Single-tenant Integration tests: https://github.com/cap-java/sdm/actions/runs/18935195679
Multi-tenant Integration tests: https://github.com/cap-java/sdm/actions/runs/18936619831

@yashmeet29 yashmeet29 requested review from rashmiangadi11 and rishikunnath2747 and removed request for rashmiangadi11 October 30, 2025 10:54
String mimeType = attachmentMetadata.get(1);
if (mimeType.equalsIgnoreCase("application/internet-shortcut")) {
int dotIndex = fileName.lastIndexOf('.');
fileName = fileName.substring(0, dotIndex);
Copy link
Collaborator

Choose a reason for hiding this comment

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

why this is removed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Earlier we were reading cmis:contentStreamFileName from DMS copyAttachments response as filename property in DB. cmis:contentStreamFileName contains .url appended in name and this name doesn't gets updated when we rename the file in DMS. That's why customer was facing the issue.

Whereas cmis:name doesn't have .url appended with the name of the link, so I have removed this part of code

@vibhutikumar07
Copy link
Collaborator

gemini review

@github-actions
Copy link
Contributor

github-actions bot commented Nov 5, 2025

Gemini Automated Review
Summary of Changes
The PR includes an update to the jacoco-maven-plugin dependency, a refactoring of CMIS property usage for filename retrieval from cmis:contentStreamFileName to cmis:name in SDMServiceImpl, and the removal of specific handling for application/internet-shortcut files in SDMCustomServiceHandler.java.

Best Practices Review 💡

  • Ensure all project dependencies are kept up-to-date to leverage bug fixes, performance improvements, and security enhancements. The jacoco-maven-plugin specifically should be at version 0.8.14.

Potential Bugs 🐛

  • SDMCustomServiceHandler.java: The removal of specific logic for application/internet-shortcut files may lead to incorrect filenames (e.g., storing with an undesired .lnk extension) if the cmis:name property now returned by the copyAttachment method does not already provide the desired filename without the shortcut extension, or if the requirement to strip this extension still exists. Comprehensive testing with application/internet-shortcut files is highly recommended to confirm behavior.

Recommendations & Required Changes 🛠️

  1. Update jacoco-maven-plugin Version: Ensure the jacoco-maven-plugin is updated to version 0.8.14 in the project's pom.xml to align with best practices for dependency management and access the latest features, bug fixes, and security patches.
    <plugin>
        <groupId>org.jacoco</groupId>
        <artifactId>jacoco-maven-plugin</artifactId>
        <version>0.8.14</version>
        <!-- Ensure other necessary configurations are present -->
    </plugin>
  2. Verify CMIS Filename Retrieval Consistency: Confirm that changing the property from cmis:contentStreamFileName to cmis:name in SDMServiceImpl consistently provides the desired filename format across all content types. This verification is crucial to prevent unexpected naming issues in various scenarios.
  3. Comprehensive Testing for application/internet-shortcut Files: Thoroughly test the handling of application/internet-shortcut files within SDMCustomServiceHandler.java.
    • Verify whether the cmis:name property now correctly provides the filename without the .lnk extension.
    • If the .lnk extension is still present and undesired, or if the requirement to strip it persists, the specific handling logic for these files should be reintroduced.

Quality Rating
7/10

Overall Assessment
The code introduces a positive dependency update and a refactoring related to CMIS property usage. However, the change in handling application/internet-shortcut files presents a potential bug that requires immediate and comprehensive verification. The jacoco-maven-plugin update is a good practice. Further testing, especially for shortcut files, and verification of CMIS property behavior are required before this PR can be merged confidently.

@yashmeet29 yashmeet29 merged commit 9d0aa9f into develop Nov 7, 2025
9 checks passed
@yashmeet29 yashmeet29 deleted the fileNameIssueInCopyafterRename branch November 7, 2025 04:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants