- Notifications
You must be signed in to change notification settings - Fork254
N°7385 - Trigger on mention executed even if it's not a mention with @#638
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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.
Review of the current proposition
Mentions are not only for Contact (@
marker by default), it can be for any kind of objects. So your current implementation breaks it for all other classes.
If we are to go further with this ticket, then we must check if the mentioned object matches the marker it begins with.
Use case that led to this PR
When pasting text from an iTop; if the text contains the hyperlink to an object with the same meta data as the mentions, then the mention will be triggered again even if the hyperlink doesn't start with the marker (@
for Contact).
@v-dumas had this behaviour recently and thought it was nice. So there is obviously a discussion to be done, so we all decide what behaviour we want.
I'm moving this PR to "Functional review" even though the technical one didn't occur yet, so the important point can be discussed before going any further. |
Functional review:
|
rquetiez commentedJan 28, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Code review : the preceding character must be dehardcoded from '@' to the one given in the configuration, depending on the object class. Also consider a multi-byte safe function. |
7334dcc
to43c1633
CompareUh oh!
There was an error while loading.Please reload this page.
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.
- Two little typographic rules to fix
- Having utilsTest derived from iTopDataTestCase seems overkill Is there an alternative?
- My mistake on my previous review: multi-byte safe function has no sense when it comes to checking the very first character of a byte sequence. Moreover, a test has been added with an emoji as the "marker".
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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.
Ok for merging
Co-authored-by: Molkobain <lajarige.guillaume@free.fr>
9a726c6
toc8fe294
Compare
Internal