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.

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

[NoQA] fix: do not use last actor details for LHN if last action is for closing chat #16606

Merged

Conversation

BeeMargarida
Copy link
Contributor

@BeeMargarida BeeMargarida commented Mar 28, 2023

Details

This a regression related to #15711. When a workspace is deleted, the preview message of the workspace channels (like #admin) appear with an incorrect message.

This PR contains a fix that only gets the last action details if the action is not related to opening or closing the chat. There are other type of actions here.

Fixed Issues

$ #15711
#15711 (comment)

Tests

No specific tests, this is a refactor of how the last message is shown in the LHN. General regression tests in this area should confirm all works fine.

  • Verify that no errors appear in the JS console

Offline tests

Not applicable

QA Steps

No specific tests, this is a refactor of how the last message is shown in the LHN. General regression tests in this area should confirm all works fine.

  • Verify that no errors appear in the JS console

PR Author Checklist

  • I linked the correct issue in the ### Fixed Issues section above
  • I wrote clear testing steps that cover the changes made in this PR
    • I added steps for local testing in the Tests section
    • I added steps for the expected offline behavior in the Offline steps section
    • I added steps for Staging and/or Production testing in the QA steps section
    • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
    • I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
  • I included screenshots or videos for tests on all platforms
  • I ran the tests on all platforms & verified they passed on:
    • Android / native
    • Android / Chrome
    • iOS / native
    • iOS / Safari
    • MacOS / Chrome / Safari
    • MacOS / Desktop
  • I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
  • I followed proper code patterns (see Reviewing the code)
    • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
      • If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
    • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
    • I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
    • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
    • I verified the JSDocs style guidelines (in STYLE.md) were followed
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I followed the guidelines as stated in the Review Guidelines
  • I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
  • I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
  • I verified that if a function's arguments changed that all usages have also been updated correctly
  • If a new component is created I verified that:
    • A similar component doesn't exist in the codebase
    • All props are defined accurately and each prop has a /** comment above it */
    • The file is named correctly
    • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
    • The only data being stored in the state is data necessary for rendering and nothing else
    • For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
    • Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
    • All JSX used for rendering exists in the render method
    • The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
  • If any new file was added I verified that:
    • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
  • If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
  • I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

Screenshots/Videos

Web image
Mobile Web - Chrome image
Mobile Web - Safari image
Desktop image
iOS image
Android image

ArekChr
ArekChr previously approved these changes Mar 28, 2023
Copy link
Contributor

@ArekChr ArekChr left a comment

Choose a reason for hiding this comment

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

LGTM:)

@BeeMargarida BeeMargarida marked this pull request as ready for review March 28, 2023 11:23
@BeeMargarida BeeMargarida requested a review from a team as a code owner March 28, 2023 11:23
@melvin-bot melvin-bot bot requested review from mountiny and parasharrajat and removed request for a team March 28, 2023 11:23
@MelvinBot
Copy link

@mountiny @parasharrajat One of you needs to 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]

Copy link
Contributor

@mountiny mountiny left a comment

Choose a reason for hiding this comment

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

Thanks, looks good, but wondering if it would be a bit cleaner with less variables, but not a blocker. @parasharrajat will you be able to test and review this one?

@parasharrajat
Copy link
Member

Yes. I will do that.

@BeeMargarida
Copy link
Contributor Author

wondering if it would be a bit cleaner with less variables, but not a blocker.

Updated!

@parasharrajat
Copy link
Member

I will be back from the dentist in 1 hour and pick this up first.

mountiny
mountiny previously approved these changes Mar 29, 2023
Copy link
Contributor

@mountiny mountiny left a comment

Choose a reason for hiding this comment

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

Looking good to me, gonna wait for Rajat's testing before merging.

Thanks @BeeMargarida great job 🎉

@parasharrajat
Copy link
Member

Just back home. Gonna look into this asap.

@mountiny
Copy link
Contributor

I hope no painful issues with your teeth 🦷

@parasharrajat
Copy link
Member

No, It is running treatment.

@parasharrajat
Copy link
Member

parasharrajat commented Mar 29, 2023

Strange, @mountiny I am not seeing the archived message in he ReportFooter on localhost.
image

This is staging
image

is it same for you?

@parasharrajat
Copy link
Member

parasharrajat commented Mar 29, 2023

So there is a hidden action that is never shown to the user but can be present.

actionName: 'REIMBURSED',

I think the best way to handle this would be the same way we display messages. report actions are filtered before being shown to the UI. So I think we should apply the same filters.

  1. First the last action should not be deprecated.
    const filteredReportActions = filterOutDeprecatedReportActions(reportActions);
  2. It should be one of the types present on the CONST.REPORT.ACTIONS.TYPE. Tests show that there can be an action of type REIMBURSED outside this list.
  3. It should not be created and Closed.
  4. It should not be deleted.

There are a few other types referring to CONST that I don't know how to handle because I don't have access to such chats.


Point 4 reminds me that It poses a challenge for us to get the correct user data from the last report actions. Deleted comments are not removed from report actions collections. So it is possible that the lastReportAction in the collection can be a deleted message. Thus we will show the wrong username with the last message. Even if we check whether the lastReportAction actor is the same as the last message actor, we won't be able to get the correct actor info.
Screenshot 2023-03-30 03:30:00

Steps
  1. On a room report.
  2. Send a message from Account B.
  3. Now log in with Account A.
  4. Send a new message from Account A.
  5. Delete the recent comment from Account A.
  6. Force the lastActorDetails to be null to test this PR. https://github.com/Expensify/App/pull/16606/files#diff-164819655552b8656799f246055951a2c8af7509d3b15063fddd328e358299bbR253

Thus this approach is not foolproof.


If we patch our solution with the following then it might work.

Instead of directly getting the lastReportAction, we somehow keep track of the filtered list or get the last action by filtering the report actions list. Then if we follow the same filtering used for the last message calculation, then the last action will always be the same as the last message in the report. This might need backend inspection to determine how the last message is calculated. I was only able to find its calculation for Comment action on front end.

@mountiny
Copy link
Contributor

regarding your concern, I think it might be because this is not synced up with main @BeeMargarida should be working in main

@BeeMargarida
Copy link
Contributor Author

I am not seeing the archived message in he ReportFooter on localhost.

This looks fixed after the merge with main.

So, just to make sure I understood correctly, there's a mismatch between the last message we are showing and the last report action (which we use to get the last message user info). When we delete a message, it seems like there is no action for a comment deletion, only for ADDCOMMENT, so if no other action happened and no personal details exist, it will show the last message actor (the one that was deleted) as the author of the last shown message (even if they are not the same).

This problem does not happen in the other place where the last action is being used because it's related to the archive reason of a chat, so no other actions occur after that.

keep track of the filtered list or get the last action by filtering the report actions list. Then if we follow the same filtering used for the last message calculation, then the last action will always be the same as the last message in the report. This might need backend inspection to determine how the last message is calculated

We have access to the action list, since the last action is calculated from that. We could check if the lastReportAction actor email (actorEmail) is the same as the one we are showing as the last message (report.lastActorEmail). If not, we can then filter the all the actions for the one that matches. Is it possible to get an ideia of how this is done on the backend?

However, I'm not sure if this solution is better than just fetch the personal details of the user if we don't have them. Is this also a possible solution?

@mountiny
Copy link
Contributor

@BeeMargarida @parasharrajat we can also just add a new filed to this onyx Selector

const lastReportActions = {};
const reportActions = {};
Onyx.connect({
key: ONYXKEYS.COLLECTION.REPORT_ACTIONS,
callback: (actions, key) => {
if (!key || !actions) {
return;
}
const reportID = CollectionUtils.extractCollectionItemID(key);
lastReportActions[reportID] = _.last(_.toArray(actions));
reportActions[key] = actions;
},
});
and pull only the last action of the types we accept

@BeeMargarida
Copy link
Contributor Author

and pull only the last action of the types we accept

I may have misunderstood, but I think it won't solve the delete comment problem. Suppose we use this approach and save in a new field called lastFilteredReportAction the last action only with type ADDCOMMENT. Using the same scenario:

  1. User 1 writes to chat
  2. We have the lastFilteredReportAction with the correct info
  3. User 1 deletes the last comment
  4. The lastFilteredReportAction will have outdated info of the comment deleted. There's no action for comment deleted.

In this case, the only solution I'm seeing is the one mentioned before of filtering the report actions for the one that matches the last message sent.

@mountiny
Copy link
Contributor

@BeeMargarida I am sorry, myabe we could bring this to the open source channel and describe the problem we are having right now summarized with steps to reproduce for example. I am not sure if I am following. If user writes a message and then deletes it, we will just show in LHN whatever was there before they sent the message as that will be the state fo the reportActions too.

@BeeMargarida
Copy link
Contributor Author

If user writes a message and then deletes it, we will just show in LHN whatever was there before they sent the message as that will be the state fo the reportActions too.

But are the deleted comments removed from the reportActions? Because the problem that I think is happening is that the reportActions, and subsequently the last report action, will point to that last deleted comment instead of the previous message.

@mountiny
Copy link
Contributor

But are the deleted comments removed from the reportActions? Because the problem that I think is happening is that the reportActions, and subsequently the last report action, will point to that last deleted comment instead of the previous message.

They are removed, at least when online and we succeed

const successData = [
{
onyxMethod: CONST.ONYX.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportID}`,
value: {
[reportActionID]: {
pendingAction: null,
previousMessage: null,
},
},
},
];
if its offline and we remove it optimistically, its still there until we get response from backend

@BeeMargarida
Copy link
Contributor Author

BeeMargarida commented Mar 31, 2023

They are removed, at least when online and we succeed

if its offline and we remove it optimistically, its still there until we get response from backend

Hum, I'll investigate this more. Monday I hope to have a better understanding and an updated proposal

@parasharrajat
Copy link
Member

parasharrajat commented Apr 6, 2023

Why is it showing no Activity for archived rooms which has a chat history? Is this expected? this is the same for staging.
image

I noticed a thing when there was an unread message on the room and the workspace got deleted, this archived room was still showing the last message with the user displayName until I clicked it. Then it started to show no activity. is this Expected? @mountiny

@parasharrajat
Copy link
Member

parasharrajat commented Apr 6, 2023

Screenshots

🔲 iOS / native

Screenshot 2023-04-07 05:40:22

🔲 iOS / Safari

Screenshot 2023-04-07 05:55:15

🔲 MacOS / Desktop

Screenshot 2023-04-07 05:57:38

🔲 MacOS / Chrome

image

🔲 Android / Chrome

🔲 Android / native

@parasharrajat
Copy link
Member

parasharrajat commented Apr 7, 2023

Although solution looks like it works for the cases where reportAction is loaded in the Onyx it is still not an optimal solution. One of the reasons could be #16606 (comment).

I am fine closing this PR with this solution but a better approach might be to get the data of actors from the backend whose info is not present in Onyx when a message is received from such an actor. I am not sure how we relate contacts with an account. But the same can be used to add an actor's data to a user's personal details store.

The logic could be:

  1. When a new message is received, Check if the user's personal details exist in the local Onyx database. If not make a call to store that actor's contact in the current user's account and also sync it in Onyx.
  2. We can use push. When a new message is created on the report, in the backend push the info of that actor to all users' accounts and then send minimal info necessary to show that actor in UI via a separate subscription. This will keep the message's subscription healthy and fast.

If this is confusing and seems like I don't know what I am talking here, then please ignore 😄 .

@parasharrajat
Copy link
Member

parasharrajat commented Apr 10, 2023

Awaiting your thoughts.

@mountiny
Copy link
Contributor

Thanks! @parasharrajat

I am not really on board for making more API calls to get the details of the person. I think that could get quite messay and for now there is not much ROI for doing this.

I think that the No Activity yet if there is a comment in the report is an issue though. @BeeMargarida Would you be able to address that?

@BeeMargarida
Copy link
Contributor Author

BeeMargarida commented Apr 11, 2023

I think that the No Activity yet if there is a comment in the report is an issue though. @BeeMargarida Would you be able to address that?

Yap, investigating that now

@BeeMargarida
Copy link
Contributor Author

BeeMargarida commented Apr 11, 2023

I noticed a thing when there was an unread message on the room and the workspace got deleted, this archived room was still showing the last message with the user displayName until I clicked it. Then it started to show no activity.

@parasharrajat Can you still replicate the issue? I've pulled from main and tried replicating that behaviour but it seems like it's working fine. For the users of a workspace chat not admin, the channels disappear once the workspace is deleted, not sure if that is what is expected.

output_1.mp4

@parasharrajat
Copy link
Member

Gonna recheck in a minute. Unfortunately, I tried yesterday but couldn't see any archived rooms not sure why.

@mountiny
Copy link
Contributor

I tested locally now and I can still reproduce although its a weird behaviour, it shows the last message for a bit and then changes to No Activity yet, maybe checking the logic for when that is shown could help us figure out why

@BeeMargarida
Copy link
Contributor Author

BeeMargarida commented Apr 12, 2023

Sorry for causing this back and forth, but I'm not able to reproduce. I've tested both in the branch and in main, and the behaviour after the steps (unread messages in chat -> delete workspace) are the same in both. What steps are you doing, exactly?

There are two weird behaviours happening in both branches:

  1. When opening an #announce archived chat, it disappears forever (can't even find it at the end of all the chats)
  2. The workspace chat shows the This workspace chat is no longer active because <chat name> is no longer an active workspace. message instead of the last one written. Don't know if this is expected behaviour.
This branch
output.mp4
Main
output_1.mp4

@mountiny
Copy link
Contributor

Wow ok thats super weird behaviour, there might have been some regression or whatnot.

@parasharrajat
Copy link
Member

I agree that I am no more able to see the archived workspaces in my account so couldn't test. For this bug to reproduce, we need to have an old archived room in which a guide messages you. Because these guides are random thus their info is not saved in your data.

@mountiny Should we go ahead and merge this PR?

Copy link
Contributor

@mountiny mountiny left a comment

Choose a reason for hiding this comment

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

@BeeMargarida One change requested.

I could not reproduce the issues locally. I think we can go ahead and merge and actually have a discussion in Slack about whats the expected out come of the preview text if we have a closed/ archived chat and handle it then based on what we agree on

@BeeMargarida
Copy link
Contributor Author

@mountiny Updated!

@mountiny
Copy link
Contributor

@parasharrajat noticed there is no reviewer checklist, can you please run though that?

Copy link
Member

@parasharrajat parasharrajat left a comment

Choose a reason for hiding this comment

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

Reviewer Checklist

  • I have verified the author checklist is complete (all boxes are checked off).
  • I verified the correct issue is linked in the ### Fixed Issues section above
  • I verified testing steps are clear and they cover the changes made in this PR
    • I verified the steps for local testing are in the Tests section
    • I verified the steps for Staging and/or Production testing are in the QA steps section
    • I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
  • I checked that screenshots or videos are included for tests on all platforms
  • I included screenshots or videos for tests on all platforms
  • I verified tests pass on all platforms & I tested again on:
    • Android / native
    • Android / Chrome
    • iOS / native
    • iOS / Safari
    • MacOS / Chrome / Safari
    • MacOS / Desktop
  • If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
  • I verified proper code patterns were followed (see Reviewing the code)
    • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
    • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
    • I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
    • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
    • I verified the JSDocs style guidelines (in STYLE.md) were followed
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I verified that this PR follows the guidelines as stated in the Review Guidelines
  • I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
  • I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
  • I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
  • If a new component is created I verified that:
    • A similar component doesn't exist in the codebase
    • All props are defined accurately and each prop has a /** comment above it */
    • The file is named correctly
    • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
    • The only data being stored in the state is data necessary for rendering and nothing else
    • For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
    • Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
    • All JSX used for rendering exists in the render method
    • The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
  • If any new file was added I verified that:
    • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
  • I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

cc: @mountiny

🎀 👀 🎀 C+ reviewed

@mountiny
Copy link
Contributor

I gave this another test based on the QA steps and I think this does not really work and the reason is that the lastMessageText is not populated for archived reports

lastMessageTextFromReport = Str.htmlDecode(report ? report.lastMessageText : '');
which then leads to the no activity yet being shown
result.alternateText = lastMessageTextFromReport.length > 0 ? lastMessageText : Localize.translate(preferredLocale, 'report.noActivityYet');

image

I will update the steps here, I think this is worthy refactor but the steps would fail QA.

In the follow up discussion we should agree what we want to show (the last message of user or the closed action text)?

@mountiny mountiny changed the title fix: do not use last actor details for LHN if last action is for closing chat [NoQA] fix: do not use last actor details for LHN if last action is for closing chat Apr 12, 2023
Copy link
Contributor

@mountiny mountiny left a comment

Choose a reason for hiding this comment

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

Thanks for the persistance here

@mountiny mountiny merged commit 2d0ef06 into Expensify:main Apr 12, 2023
@OSBotify
Copy link
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by https://github.com/mountiny in version: 1.3.0-0 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@OSBotify
Copy link
Contributor

🚀 Deployed to production by https://github.com/mountiny in version: 1.3.0-2 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

// The report is only visible if it is the last action not deleted that
// does not match a closed or created state.
const reportActionsForDisplay = _.filter(actionsArray, (reportAction, actionKey) => (ReportActionsUtils.shouldReportActionBeVisible(reportAction, actionKey)
&& (reportAction.actionName !== CONST.REPORT.ACTIONS.TYPE.CREATED)));
Copy link
Member

Choose a reason for hiding this comment

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

Neither a regression nor in the scope of this PR, but we should also filter delete pending action items to correctly get the last visible item.

#24231

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants