Fix copy invite room message#30898
Conversation
|
@cubuspl42 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
@cubuspl42 I updated, please help to review again. |
| * @param {Object} reportAction | ||
| * @returns {String} | ||
| */ | ||
| function getRoomChannelLogMemberMessage(reportAction) { |
There was a problem hiding this comment.
Looks better! But I have a suggestion to increase the simplicity of the function. Let me know if you agree.
/**
* Return room channel log display message
*
* @param {Object} reportAction
* @returns {String}
*/
function getRoomChannelLogMemberMessage(reportAction) {
const actionPerformed = reportAction.actionName === CONST.REPORT.ACTIONS.TYPE.ROOMCHANGELOG.INVITE_TO_ROOM ? 'invited' : 'removed';
const mentions = _.map(reportAction.originalMessage.targetAccountIDs, (accountID) => {
const personalDetail = lodashGet(allPersonalDetails, accountID);
const displayNameOrLogin =
LocalePhoneNumber.formatPhoneNumber(lodashGet(personalDetail, 'login', '')) || lodashGet(personalDetail, 'displayName', '') || Localize.translateLocal('common.hidden');
return `@${displayNameOrLogin}`;
});
const lastMention = mentions.pop();
if (mentions.length === 0) {
return `${actionPerformed} ${lastMention}`;
}
return `${actionPerformed} ${mentions.join(', ')} and ${lastMention}`;
}There was a problem hiding this comment.
Look good to me. Will update soon.
| if (listMention.length === 1) { | ||
| lastPrefix = ' and '; | ||
|
|
||
| if (mentions.length === 1) { |
There was a problem hiding this comment.
Have you actually tested that this case is required? I thought I tested it with my suggested variant
There was a problem hiding this comment.
Oh... I think I get it
There was a problem hiding this comment.
Still, I think this logic is still slightly more readable (even if the advantage is smaller than I originally assumed)
|
Bad news: I think we'll need to expand the scope of the PR to handle another variant of the In the
|
|
I think we can do that in the function of this PR. |
|
@dukenv0307 Great! We'll need to expand the testing steps. When you do this, please re-upload at least the Web ("MacOS: Chrome / Safari") platform video. |
|
@cubuspl42 Updated to fix copy policy log and rename the variable to the same as we used in |
|
I'm having trouble observing the invite logs in the In one case, I got information about one user being invited to a room when I invited three users at once: In another case, I got no update about inviting to a room. Do you always get reliable logs in |
|
@cubuspl42 Can you share a video for this case? |
admin-log-issue-web.mp4
...but in
I'm not saying this is something your PR introduced, but it complicates the testing 🫤 |
|
@cubuspl42 I tested sometimes and didn't face this issue. |
|
@dukenv0307 I'll re-test |
Reviewer Checklist
Screenshots/VideosWebinvited-members-copy-to-clipboard-web.mp4Mobile Web - Chromeinvited-members-copy-to-clipboard-android-web-compressed.mp4Mobile Web - Safariinvited-members-copy-to-clipboard-ios-web.mp4DesktopiOSinvited-members-copy-to-clipboard-ios.mp4Androidinvited-members-copy-to-clipboard-android-compressed.mp4 |
|
@cubuspl42 Is this PR already to approve? |
|
@dukenv0307 Thank you for reminding me. I submitted the checklist without approving. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to production by https://github.com/luacmartins in version: 1.4.1-13 🚀
|
| reportAction.actionName === CONST.REPORT.ACTIONS.TYPE.ROOMCHANGELOG.INVITE_TO_ROOM || reportAction.actionName === CONST.REPORT.ACTIONS.TYPE.POLICYCHANGELOG.INVITE_TO_ROOM | ||
| ? ' to' | ||
| : ' from'; | ||
| message += `${preposition} ${roomName}`; |
There was a problem hiding this comment.
Minor bug from here, the room name was not shown as link. More details here #31568
5 participants
Details
Fix copy invite room message
Fixed Issues
$ #30272
PROPOSAL: #30272 (comment)
Tests
Offline tests
None
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Screen.Recording.2023-11-06.at.11.55.59.mov
Screen.Recording.2023-11-08.at.15.57.58.mov
Android: mWeb Chrome
Screen.Recording.2023-11-06.at.11.27.16.mov
iOS: Native
Screen.Recording.2023-11-06.at.11.45.59.mov
iOS: mWeb Safari
Screen.Recording.2023-11-06.at.11.32.40.mov
MacOS: Chrome / Safari
Screen.Recording.2023-11-06.at.11.17.22.mov
MacOS: Desktop
Screen.Recording.2023-11-06.at.12.01.39.mov