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

In FF! keep one character more when truncating then in Ladybug #6762

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

mhdirkse
Copy link
Contributor

@mhdirkse mhdirkse commented May 3, 2024

Both the Frank!Framework's ladybug module and ladybug itself truncate messages when they are too long. Because the F!F does the truncation first, Ladybug does not see that truncation was necessary. Because of this, Ladybug did not report that a report message was truncated.

This PR lets the F!F keep one extra character such that Ladybug does reduce the message size when truncation is needed. With this PR, the old Ladybug GUI does show it when truncation has been done:

messageTruncatedOldLadybug

The new Ladybug GUI does not show truncation even with the present PR included in the F!F, see:

messageTruncatedNewLadybug

@mhdirkse mhdirkse requested a review from jacodg May 3, 2024 16:10
@philipsens philipsens changed the title In f-f, keep one character more when truncating then in Ladybug In FF! keep one character more when truncating then in Ladybug May 3, 2024
@mhdirkse
Copy link
Contributor Author

mhdirkse commented May 3, 2024

Ik zie een bot melding over code coverage. Als deze code wordt gemerged, dan kan ik later zorgen voor een integratie test in ladybug-ff-cypress-test. Is dit de moeite waard en heeft het prioriteit?

@nielsm5
Copy link
Sponsor Member

nielsm5 commented May 4, 2024

Als de ladybug maxSize +1 krijgt dan weet de ladybug toch niet hoe groot het bericht was? Ik zie in het plaats '55 characters removed' staan. Ik neem aan dat dat dan altijd '1 characters removed' zal worden?

// Both Ladybug and the present class in the Frank!Framework
// truncate messages as configured through maxMessageLength.
// In this class we keep one character more such that
// Ladybug can detect when a message is being truncated.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add to the comment something like the following?

and show a message to the user that the message has been truncated

// Both Ladybug and the present class in the Frank!Framework
// truncate messages as configured through maxMessageLength.
// In this class we keep one character more such that
// Ladybug can detect when a message is being truncated.
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice to not duplicate the comment in two classes. A possible solution would be to add something like the following with the comment to one of the classes and use this var in both classes:

protected static final long MAX_MESSAGE_LENGTH_ADJUSTMENT = 1L;

// In this class we keep one character more such that
// Ladybug can detect when a message is being truncated.
private int truncateMessageToLength = -1;

Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of this extra var and method you could do "maxMessageLengt + MAX_MESSAGE_LENGTH_ADJUSTMENT" where you now used truncateMessageToLength (not sure what would be the best solution)

@mhdirkse
Copy link
Contributor Author

mhdirkse commented May 6, 2024

Sorry, I see now that I did not test correctly earlier. See picture below:

messageTruncatedOldLadybug_output

Earlier I checked the input message of the adapter, which is the filename. That one showed a reasonable value for the number of truncated characters. But I had to look at the output, the result of reading the file. That one shows that one only one character is truncated. It turns out that this fix does not work.

Copy link

sonarcloud bot commented May 6, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@nielsm5 nielsm5 marked this pull request as draft May 7, 2024 11:30
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.

None yet

3 participants