N°7385 - Trigger on mention executed even if it's not a mention with @ #638
Conversation
There was a problem hiding this comment.
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:
|
|
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. |
accognet
force-pushed
the
feature/7385-on_mention
branch
from
7334dcc to
43c1633
Compare
Co-authored-by: Molkobain <[email protected]>
There was a problem hiding this comment.
- 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".
application/utils.inc.php
Outdated
| continue; | ||
| } | ||
| //tests if the name starts with $sMentionPrefix (e.g. '@' for 'Contact' class) | ||
| if (str_starts_with($sMatchedName,$sMentionPrefix) === false) { |
There was a problem hiding this comment.
typographic rules: a space is expected after a coma
| { | ||
| const USE_TRANSACTION = false; |
There was a problem hiding this comment.
This statement is a trap in the future: adding a test altering the data will leave the DB changed.
Disabling transaction is required here because there is a "data provider" that invokes setUp(), causing a false negative error "two many transactions left open". This issue was revealed when the class inheritance was changed from ItopTestCase to ItopDataTestCase.
Solution:
- Rework the current proposal to move the new tests into a dedicated test class derived from
ItopDataTestCase, and leave the existing test class unchanged (derived fromItopTestCase) - Fix the existing test class will be done in a separate flow
| @@ -653,6 +658,7 @@ public function ToAcronymProvider() | |||
| */ | |||
| public function testGetMentionedObjectsFromText($sInput, $aExceptedMentionedObjects) | |||
| { | |||
| MetaModel::GetConfig()->Set('mentions.allowed_classes', ['@' => 'Person','😊#' => 'Team']); | |||
There was a problem hiding this comment.
typographic rules: a space is expected after a coma
Internal