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

Update beginningOfChatHistory with fresh copy #47427 #49209

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

mariapeever
Copy link

@mariapeever mariapeever commented Sep 14, 2024

Details

  • Updates beginningOfChatHistory with fresh copy #47427
  • Includes updates to the copy for the conceirge chat, user chat, group chat, workspace chat, #announce room, #admins room, domain room, user-created room and invoice room.

Fixed Issues

$ #47427
PROPOSAL: #47427

Tests

  1. Start a Concierge chat
  2. Start a DM chat with a random user
  3. Validate that the beginning of chat history matches the new copy
  4. Start a group chat with >= 1 other users
  5. Start a new Workspace chat
  6. Open the #announce room from Workspaces > announce room
  7. Open the #admins room
  8. Open a domain room
  9. Create a user room
  10. Create an invoice room
  • Verify that the beginning of all chat histories matches with the new copy

Offline tests

Tests as above - no changes

QA Steps

As above

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: mWeb Chrome
    • iOS: Native
    • iOS: mWeb 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 the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
    • 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 grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
    • 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 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(theme.componentBG))
  • If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
  • 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 the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
  • If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
    • I verified that all the inputs inside a form are aligned with each other.
    • I added Design label and/or tagged @Expensify/design so the design team can review the changes.
  • 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.

Screenshots/Videos

Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari Screenshot 2024-09-16 at 09 38 53 Screenshot 2024-09-16 at 09 39 19 Screenshot 2024-09-16 at 09 40 01 Screenshot 2024-09-16 at 09 40 17 Screenshot 2024-09-16 at 09 40 40
MacOS: Desktop

Copy link
Contributor

github-actions bot commented Sep 14, 2024

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@mariapeever
Copy link
Author

I have read the CLA Document and I hereby sign the CLA.

@mariapeever mariapeever marked this pull request as ready for review September 16, 2024 09:02
@mariapeever mariapeever requested a review from a team as a code owner September 16, 2024 09:02
@melvin-bot melvin-bot bot requested a review from ikevin127 September 16, 2024 09:02
Copy link

melvin-bot bot commented Sep 16, 2024

@ikevin127 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]

@melvin-bot melvin-bot bot removed the request for review from a team September 16, 2024 09:02
@ikevin127
Copy link
Contributor

♻️ I started working on the checklist.

@mariapeever In the meantime please make sure to add screenshots or videos for all platforms as this is a blocker for the PR because of the checklist:

  • I ran the tests on all platforms & verified they passed on:
  • I Android: Native
  • I Android: mWeb Chrome
  • I iOS: Native
  • I iOS: mWeb Safari
  • I MacOS: Chrome / Safari
  • I MacOS: Desktop

Another blocker for this PR is the fact that while english languace copy was changed, I noticed that the spanish translations were not updated, as per this checklist item:

  • 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:

Meaning for all english copy that was changed, we need to also translate and change the spanish version of the same variables - to do this, as mentioned in the checklist: you need to post in #expensify-open-source Slack channel and request translations for the new english copy and provide some context on the request like link to the issue.

@ikevin127
Copy link
Contributor

Meaning for all english copy that was changed, we need to also translate and change the spanish version of the same variables - to do this, as mentioned in the checklist: you need to post in #expensify-open-source Slack channel and request translations for the new english copy and provide some context on the request like link to the issue.

Regarding ^ this, I asked in slack for somebody to help w/ spanish translations: https://expensify.slack.com/archives/C01GTK53T8Q/p1726624473137749

@mariapeever ⚠️ Today I continued with the testing and, besides the things mentioned in #49209 (comment), I found a few issues with the PR which would lead to regressions post-merge, therefore they need to be addressed before I can move forward and complete the PR Reviewer Checklist and Approve:

  • 1. Invoice room copy is incomplete, missing Use the + button to send an invoice. part (see first part of the video)

    • a. If Invoice Room description is added, the copy is reduced to the names only (see second part of the video)
    • these two issues need to be addressed, especially point (a.) as they should be matching the current behaviour from staging when Room description is added
      .
    Video
    invoice-issues.mov

Important

Same issue with changing the description happens in the following cases as well: workspace #announce and #admins rooms, so they should be fixed / tested there as well.

  • 2. Invoice room copy is wrong after the workspace is deleted (archived)
    • below I attached pictures of this PR vs Staging for comparison, we can notice that on the PR the variable is misplaced and it's missing the (archived) part if compared to staging
      .
PR Staging
PR staging

@mariapeever
Copy link
Author

@ikevin127 I have resolved the issues and updated the PR. Just waiting for a Spanish translation.

@ikevin127
Copy link
Contributor

Just got the verified spanish translations:

  1. This chat is with Concierge. Ask questions and get 24/7 realtime support. / Este chat es con Concierge. Haz preguntas y obtén soporte en tiempo real las 24/7.
  2. This chat is with [recipient]. Use the + button to split, submit, or pay an expense. / Este chat es con [recipient]. Usa el botón + para dividir, enviar o pagar un gasto.
  3. This chat is with [recipients that don't include you]. Use the + button to split an expense. / Este chat es con [recipients that don't include you]. Usa el botón + para dividir un gasto.
  4. This is where [submitter] will submit expenses to the [workspaceName] workspace. Just use the + button. / Aquí es donde [submitter] enviará los gastos al espacio de trabajo [workspaceName]. Solo usa el botón +.
  5. This chat is with everyone in the [workspaceName] workspace. Use it for the most important announcements. / Este chat es con todos en el espacio de trabajo [workspaceName]. Úsalo para los anuncios más importantes.
  6. This chat is with [workspaceName] workspace admins. Use it to chat about workspace setup and more. / Este chat es con los administradores del espacio de trabajo [workspaceName]. Úsalo para hablar sobre la configuración del espacio de trabajo y más.
  7. This chat is with all Expensify members on the [domain] domain. Use it to chat with colleagues, share tips, and ask questions. / Este chat es con todos los miembros de Expensify en el dominio [domain]. Úsalo para chatear con compañeros de trabajo, compartir consejos y hacer preguntas.
  8. This chat room is for anything #[roomName] related. It was created by [displayName]. / Este chat es para todo lo relacionado con #[roomName]. Fue creado por [displayName].
  9. This chat is for invoices between [invoice sender/bill payer workspaceName] and [invoice/paid bill receiver workspaceName OR displayName]. Use the + button to send an invoice. / Este chat es para facturas entre [invoice sender/bill payer workspaceName] y [invoice/paid bill receiver workspaceName OR displayName]. Usa el botón + para enviar una factura.

@mariapeever ⚠️ Regarding the recent commits, while they did solve 1 instance of the reported issue, I'm afraid they broke things in other parts of the logic:

Important

When testhing this, please compare PR / Staging side by side for all 9 copy cases and also for regular and workspace linked chats which also have the workspace archived version once a workspace is deleted.

I would suggest looking at the current staging code and applying the changes to simply change the copy and not the logic because currently it feels like we're drifting away more and more from current Staging with each change and we're getting more issues.

Here's a summary after the latest changes (3 commits):

  • 1. Invoice room copy is incomplete, missing Use the + button to send an invoice. part (see first part of the video)
    • a. If Invoice Room description is added, the copy is reduced to the names only (see second part of the video)
  • 2. Invoice room copy is wrong after the workspace is deleted (archived)
    • these three issues were fixed ✅

⚠️ The following issues surfaced after the latest changes:

Workspace admins - Displaying #[roomName] instead of [workspaceName]

PR Staging
workspace-admins workspace-admins-staging

Workspace announce - Displaying #[roomName] instead of [workspaceName]

PR Staging
Screenshot 2024-09-18 at 13 52 07 workspace-announce-staging

Workspace archived - Missing [submitter]

PR Staging
ws-archived ws-archived-staging

Workspace announce archived - Missing [submitter]

PR Staging
ws-announce-archived-staging we-announce-archived

Workspace room archived - Crashing the app when opened

room-archived-crash

@mariapeever
Copy link
Author

@ikevin127 I have made the adjustments and added the Spanish translation.

@ikevin127
Copy link
Contributor

Unfortunately, as I mentioned before we're drifting away more and more from current staging with each change and we're getting more issues with each new commit.

Here are a few PR / Staging screenshots to showcase most of the current issues (and there are more):

Screenshots

Workspace - #admins

PR Staging
admins admins-stg

Workspace - #announce

PR Staging
announce announce-stg

Workspace - #admins (archived)

PR Staging
ws-admins-archived ws-admins-archived-stg

Workspace - #announce (archived)

PR Staging
ws-announce-archived ws-announce-archived-stg

Workspace (archived)

PR Staging
ws-archived ws-archived-stg

Workspace - Member Chat (archived)

PR Staging
ws-archived-member ws-archived-member-stg

Invoice room

PR Staging
invoice-chat invoice-chat-stg

Invoice room (archived)

PR Staging
invoice-archived invoice-archived-stg

I have to take some accountability here for assignment and admit that I did not think this issue / PR will be as complex as it is in terms of code changes and now it's becoming clear to me that I did not take in consideration the codebase familiarity when I assigned you given that this is your very first contribution.

Given that it's been over 1 week since the PR was opened and we're not even close to merge and also some of the feedback I provided in the IMPORTANT notes above was not taken in consideration - I discussed with the team and decided that given the timeline of the issue, we're going to re-assign this to the contributor next in line given they have more experience with the codebase.

cc @mariapeever

@mariapeever
Copy link
Author

@ikevin127 I haven't seen your last message - I was just working on the PR with the screenshots until today. I don't think this makes sense now considering the time I have spent to submit the updates. You probably thought it could take longer. I have re-tested everything and it works.

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.

2 participants