- Notifications
You must be signed in to change notification settings - Fork244
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
N°7385 - Trigger on mention executed even if it's not a mention with @#638
base:support/3.2
Are you sure you want to change the base?
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:
|
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 to43c1633CompareCo-authored-by: Molkobain <lajarige.guillaume@free.fr>
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".
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
typographic rules: a space is expected after a coma
| { | ||
| const USE_TRANSACTION = false; |
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.
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 fromItopTestCase toItopDataTestCase.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
typographic rules: a space is expected after a coma
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
Internal