-
Notifications
You must be signed in to change notification settings - Fork 275
Allowing E-Document Attachments to be PDF #5311
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?
Conversation
src/Apps/W1/EDocument/App/src/Service/EDocumentService.Table.al
Outdated
Show resolved
Hide resolved
| #endif | ||
| end; | ||
|
|
||
| procedure GetFileExtension() FileExtension: Text |
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.
I understand that we need a solution that will work pretty soon.
I can live with this but I would still prefer some solution that will store the file extension at the e-document log table.
Therefore I would suggest to rename this procedure and the event to something like GetDefaultFileExtension.
In the future this can be improved that one will use the file extension of a e-document log when it's set and otherwise the default file extension for this service.
Another solution:
Alternatively I would suggest to instead refactor the event in the E-Document Log.
https://github.com/microsoft/BCApps/blob/main/src/Apps/W1/EDocument/App/src/Logging/EDocumentLog.Table.al
And extract the File extension from there.
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.
I fully agree that having the file extension stored in the Log table would make things much easier and more consistent. I personally would love it. However, introducing that change is outside the scope of this pull request and could also impact existing Microsoft connectors that rely on the current behavior. For now, I think your suggestion to rename the procedure to GetDefaultFileExtension is a good fit for this scope, and I’ll make that adjustment. I also find this name much better.
Regarding the idea of refactoring the OnBeforeExportDataStorage event: if the file extension were available in the Log table, this IntegrationEvent would likely not be necessary at all. After reviewing its usage, I noticed it’s also leveraged by OneDrive, SharePoint, and Outlook integrations, which makes it clear that this is an area where Microsoft would need to drive the change. For now, I’ll keep the current approach aligned with the existing design.
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.
I understand that some parts are out of the scope of this PR, but I appreciate the name change. Thank you.
src/Apps/W1/EDocument/App/src/Processing/EDocumentProcessing.Codeunit.al
Outdated
Show resolved
Hide resolved
|
Looks like we've got another codecop issue: Possible overflow assigning 'Text' to 'Text[4]'. |
@miljance the problem seems to be the parametere I would refactor this procedure, to only return a AttachmentFileNameWithExtension with a Max Length of 250 (This is the maximium length in the BaseApp). I had a similiar requirement in the past to trim a filename: // This shortens the filename but keeps the file extension.
procedure ShortenFileName(FileName: Text): Text[250]
var
FileManagement: Codeunit "File Management";
MaxFileNameWithoutExtensionLength: Integer;
Extension: Text;
FileNameWithoutExtension: Text;
FileNameWithFixedLength: Text[250];
begin
if StrLen(FileName) <= MaxStrLen(FileNameWithFixedLength) then begin
FileNameWithFixedLength := CopyStr(FileName, 1, MaxStrLen(FileNameWithFixedLength));
exit(FileNameWithFixedLength);
end;
FileNameWithoutExtension := FileManagement.GetFileNameWithoutExtension(FileName);
Extension := FileManagement.GetExtension(FileName);
MaxFileNameWithoutExtensionLength := MaxStrLen(FileNameWithFixedLength) - StrLen(Extension) - 1;
FileNameWithFixedLength := CopyStr(FileNameWithoutExtension, 1, MaxFileNameWithoutExtensionLength) + '.' + Extension;
exit(FileNameWithFixedLength);
end; |
cde3758 to
8062f05
Compare
8062f05 to
045a9c8
Compare
The overflow comes from using Text[4] for AttachmentFileExtension in CreateAttachmentsBlob. @djukicmilica or @JesperSchulz please let me know if you think it makes sense to deal with file name length limitation in the scope of this change. |
Summary
Work Item(s)
Fixes #5220 and #5104
Fixes AB#611441