-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Autolink when edititng comments except wehn explicit link removal #13551
Merged
Merged
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
2bcc19d
fix autolinking issue
markarmanus ee9db5d
add remaining code
markarmanus 64c91da
addrss comments
markarmanus 63a277a
add unit tests
markarmanus 89fd576
remove only
markarmanus 90a79a6
add code polish based on comments
markarmanus 44f5d8c
Merge branch 'main' into linkWhenEditingComment
markarmanus 6f17970
change reg to regex
markarmanus a76c8c3
change l to L
markarmanus File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -892,6 +892,68 @@ function deleteReportComment(reportID, reportAction) { | |
API.write('DeleteComment', parameters, {optimisticData, successData, failureData}); | ||
} | ||
|
||
/** | ||
* @param {String} comment | ||
* @returns {Array} | ||
*/ | ||
const extractLinksInMarkdownComment = (comment) => { | ||
const regex = /\[[^[\]]*\]\(([^()]*)\)/gm; | ||
const matches = [...comment.matchAll(regex)]; | ||
|
||
// Element 1 from match is the regex group if it exists which contains the link URLs | ||
const links = _.map(matches, match => match[1]); | ||
return links; | ||
}; | ||
|
||
/** | ||
* Compares two markdown comments and returns a list of the links removed in a new comment. | ||
* | ||
* @param {String} oldComment | ||
* @param {String} newComment | ||
* @returns {Array} | ||
*/ | ||
const getRemovedMarkdownLinks = (oldComment, newComment) => { | ||
const linksInOld = extractLinksInMarkdownComment(oldComment); | ||
const linksInNew = extractLinksInMarkdownComment(newComment); | ||
return _.difference(linksInOld, linksInNew); | ||
}; | ||
|
||
/** | ||
* Removes the links in a markdown comment. | ||
* example: | ||
* comment="test [link](https://www.google.com) test", | ||
* links=["https://www.google.com"] | ||
* returns: "test link test" | ||
* @param {String} comment | ||
* @param {Array} links | ||
* @returns {String} | ||
*/ | ||
const removeLinks = (comment, links) => { | ||
let commentCopy = comment.slice(); | ||
_.forEach(links, (link) => { | ||
const regex = new RegExp(`\\[([^\\[\\]]*)\\]\\(${link}\\)`, 'gm'); | ||
const linkMatch = regex.exec(commentCopy); | ||
const linkText = linkMatch && linkMatch[1]; | ||
commentCopy = commentCopy.replace(`[${linkText}](${link})`, linkText); | ||
}); | ||
return commentCopy; | ||
}; | ||
|
||
/** | ||
* This function will handle removing only links that were purposely removed by the user while editing. | ||
* @param {String} newCommentText text of the comment after editing. | ||
* @param {Array} originalHtml original html of the comment before editing | ||
* @returns {String} | ||
*/ | ||
const handleUserDeletedLinks = (newCommentText, originalHtml) => { | ||
const parser = new ExpensiMark(); | ||
const htmlWithAutoLinks = parser.replace(newCommentText); | ||
const markdownWithAutoLinks = parser.htmlToMarkdown(htmlWithAutoLinks); | ||
const markdownOriginalComment = parser.htmlToMarkdown(originalHtml); | ||
const removedLinks = getRemovedMarkdownLinks(markdownOriginalComment, newCommentText); | ||
return removeLinks(markdownWithAutoLinks, removedLinks); | ||
}; | ||
|
||
/** | ||
* Saves a new message for a comment. Marks the comment as edited, which will be reflected in the UI. | ||
* | ||
|
@@ -904,9 +966,13 @@ function editReportComment(reportID, originalReportAction, textForNewComment) { | |
|
||
// Do not autolink if someone explicitly tries to remove a link from message. | ||
// https://github.com/Expensify/App/issues/9090 | ||
// https://github.com/Expensify/App/issues/13221 | ||
const originalCommentHTML = lodashGet(originalReportAction, 'message[0].html'); | ||
const markdownForNewComment = handleUserDeletedLinks(textForNewComment, originalCommentHTML); | ||
|
||
const autolinkFilter = {filterRules: _.filter(_.pluck(parser.rules, 'name'), name => name !== 'autolink')}; | ||
const htmlForNewComment = parser.replace(textForNewComment, autolinkFilter); | ||
const originalMessageHTML = parser.replace(originalReportAction.message[0].html, autolinkFilter); | ||
const htmlForNewComment = parser.replace(markdownForNewComment, autolinkFilter); | ||
const parsedOriginalCommentHTML = parser.replace(originalCommentHTML, autolinkFilter); | ||
|
||
// Delete the comment if it's empty | ||
if (_.isEmpty(htmlForNewComment)) { | ||
|
@@ -915,7 +981,7 @@ function editReportComment(reportID, originalReportAction, textForNewComment) { | |
} | ||
|
||
// Skip the Edit if message is not changed | ||
if (originalMessageHTML === htmlForNewComment.trim()) { | ||
if (parsedOriginalCommentHTML === htmlForNewComment.trim()) { | ||
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. There was a case where resaving the edited message without changing the content will parse the link again #29225. Instead, it should not auto-link when it is removed explicitly and the content is not changed. |
||
return; | ||
} | ||
|
||
|
@@ -927,7 +993,7 @@ function editReportComment(reportID, originalReportAction, textForNewComment) { | |
message: [{ | ||
isEdited: true, | ||
html: htmlForNewComment, | ||
text: textForNewComment, | ||
text: markdownForNewComment, | ||
type: originalReportAction.message[0].type, | ||
}], | ||
}, | ||
|
@@ -1362,6 +1428,7 @@ export { | |
broadcastUserIsTyping, | ||
togglePinnedState, | ||
editReportComment, | ||
handleUserDeletedLinks, | ||
saveReportActionDraft, | ||
deleteReportComment, | ||
getSimplifiedIOUReport, | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Coming from #18911:
I understand it's difficult to find edge cases but this regex didn't handle such cases like
[[test]](test.com)
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.
For consistency, we're now using markdown link regex exported from
expensify-common
.