Skip to content
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.

Sign up for GitHub

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

Show Room name in Desktop notification title if message is come from a chat room #19053

Merged
merged 4 commits into from
May 18, 2023
Merged

Show Room name in Desktop notification title if message is come from a chat room #19053

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3e4762e
Show roomName in notif title if message come from a chat room
hoangzinh May 16, 2023
709b63d
Build web notification title & body if message is come from chatroom
hoangzinh May 17, 2023
dc63f9b
Refactor code
hoangzinh May 17, 2023
33e560c
Add empty lines to make code look better
hoangzinh May 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 18 additions & 3 deletions src/libs/Notification/LocalNotification/BrowserNotifications.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import _ from 'underscore';
import focusApp from './focusApp';
import * as AppUpdate from '../../actions/AppUpdate';
import EXPENSIFY_ICON_URL from '../../../../assets/images/expensify-logo-round-clearspace.png';
import * as ReportUtils from '../../ReportUtils';

const DEFAULT_DELAY = 4000;

Expand Down Expand Up @@ -92,20 +93,34 @@ export default {
* Create a report comment notification
*
* @param {Object} params
* @param {Object} params.report
* @param {Object} params.reportAction
* @param {Function} params.onClick
* @param {Boolean} usesIcon true if notification uses right circular icon
*/
pushReportCommentNotification({reportAction, onClick}, usesIcon = false) {
pushReportCommentNotification({report, reportAction, onClick}, usesIcon = false) {
const {person, message} = reportAction;
const plainTextPerson = _.map(person, (f) => f.text).join();
const isChatRoom = ReportUtils.isChatRoom(report);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NAB. Can you move this line to look like this just for clarity

        const isChatRoom = ReportUtils.isChatRoom(report);

        let title;
        let body;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it a good idea if I move them to top? Basically, intepreter usually move declaration to the top too.

        let title;
        let body;

        const isChatRoom = ReportUtils.isChatRoom(report);
        const {person, message} = reportAction;
        const plainTextPerson = _.map(person, (f) => f.text).join();
        // Specifically target the comment part of the message
        const plainTextMessage = (_.find(message, (f) => f.type === 'COMMENT') || {}).text;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes looks good. but add an empty line after isChatRoom and before the comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

pushReportCommentNotification({report, reportAction, onClick}, usesIcon = false) {
    let title;
    let body;

    const isChatRoom = ReportUtils.isChatRoom(report);

    const {person, message} = reportAction;
    const plainTextPerson = _.map(person, (f) => f.text).join();

    // Specifically target the comment part of the message
    const plainTextMessage = (_.find(message, (f) => f.type === 'COMMENT') || {}).text;

    if (isChatRoom) {
        const roomName = _.get(report, 'displayName', '');
        title = roomName;
        body = `${plainTextPerson}: ${plainTextMessage}`;
    } else {
        title = plainTextPerson;
        body = plainTextMessage;
    }

    push({
        title,
        body,
        delay: 0,
        onClick,
        icon: usesIcon ? EXPENSIFY_ICON_URL : '',
    });
},

Just wanna clarify 1 more question. Do you think it too much empty line in this function? Or it looks good

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me 😁

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have updated PR with our discussion above. Could you help to review them again? Thanks


// Specifically target the comment part of the message
const plainTextMessage = (_.find(message, (f) => f.type === 'COMMENT') || {}).text;

let title = '';
let body = '';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let title = '';
let body = '';
let title;
let body;

NAB. No need to set a init value here.


if (isChatRoom) {
const roomName = _.get(report, 'displayName', '');
Copy link
Contributor

@techievivek techievivek Oct 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be reportName? Because I can't see the displayName property. I was working on this PR #28200 (comment) and realized that the notifications for chat rooms might be broken for web and desktop?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a regression from #21406 #21406 (comment). This should be changed to:

const roomName = ReportUtils.getReportName(report);

But if reportName prop already exists we can use it directly.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, great, thanks for the context. I will update it to use getReportName.

title = roomName;
body = `${plainTextPerson}: ${plainTextMessage}`;
} else {
title = plainTextPerson;
body = plainTextMessage;
}

push({
title: plainTextPerson,
body: plainTextMessage,
title,
body,
delay: 0,
onClick,
icon: usesIcon ? EXPENSIFY_ICON_URL : '',
Expand Down
4 changes: 2 additions & 2 deletions src/libs/Notification/LocalNotification/index.desktop.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import BrowserNotifications from './BrowserNotifications';

function showCommentNotification({reportAction, onClick}) {
BrowserNotifications.pushReportCommentNotification({reportAction, onClick});
function showCommentNotification({report, reportAction, onClick}) {
BrowserNotifications.pushReportCommentNotification({report, reportAction, onClick});
}

function showUpdateAvailableNotification() {
Expand Down
4 changes: 2 additions & 2 deletions src/libs/Notification/LocalNotification/index.website.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import BrowserNotifications from './BrowserNotifications';

function showCommentNotification({reportAction, onClick}) {
BrowserNotifications.pushReportCommentNotification({reportAction, onClick}, true);
function showCommentNotification({report, reportAction, onClick}) {
BrowserNotifications.pushReportCommentNotification({report, reportAction, onClick}, true);
}

function showUpdateAvailableNotification() {
Expand Down
1 change: 1 addition & 0 deletions src/libs/actions/Report.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1378,6 +1378,7 @@ function showReportActionNotification(reportID, action) {

Log.info('[LocalNotification] Creating notification');
LocalNotification.showCommentNotification({
report: allReports[reportID],
reportAction: action,
onClick: () => {
// Navigate to this report onClick
Expand Down