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

Automatically pin the newly created #admins chat #14907

Merged
merged 3 commits into from
Feb 23, 2023
Merged

Conversation

mountiny
Copy link
Contributor

@mountiny mountiny commented Feb 7, 2023

Details

When workspace is created, make sure the admins chat is automatically pinned.

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/258515

Tests

  • Verify that no errors appear in the JS console
  1. Create workspace
  2. Make sure the created #admins chat is pinned in the LHN
  3. In OldDot, add another admin to the workspace
  4. Log into the account of the new admin
  5. Verify the #admins room is pinned in their LHN

Offline tests

  1. Go offline
  2. Create the workspace
  3. Make sure the optimistically create admins chat room is pinned in the LHN

QA Steps

Same as tests.

  • 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.
  • I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

Screenshots/Videos

Web
14907web.mp4
Mobile Web - Chrome
14907androidMweb.mp4
Mobile Web - Safari
14907iosweb.mp4
Desktop
14907desktop.mp4
iOS
14907ios.mp4
Android
14907android.mp4

@mountiny mountiny self-assigned this Feb 7, 2023
@mountiny mountiny marked this pull request as ready for review February 22, 2023 22:47
@mountiny mountiny requested a review from a team as a code owner February 22, 2023 22:47
@melvin-bot melvin-bot bot requested review from thesahindia and thienlnam and removed request for a team February 22, 2023 22:48
@MelvinBot
Copy link

@thesahindia @thienlnam 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]

@thienlnam
Copy link
Contributor

Changes look fine - will leave to @thesahindia to run through checklist items

@thesahindia
Copy link
Member

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.
  • 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 reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web
Screen.Recording.2023-02-23.at.4.17.44.PM.mov
Mobile Web - Chrome
Screen.Recording.2023-02-23.at.4.20.52.PM.mov
Mobile Web - Safari
Screen.Recording.2023-02-23.at.4.30.20.PM.mov
Desktop
Screen.Recording.2023-02-23.at.4.39.11.PM.mov
iOS
Screen.Recording.2023-02-23.at.4.28.34.PM.mov
Android
Screen.Recording.2023-02-23.at.4.24.43.PM.mov

Copy link
Member

@thesahindia thesahindia left a comment

Choose a reason for hiding this comment

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

I verified that admins room gets pinned for the creator but I couldn't test the same for the newly added member (I think I can't add someone as an admin or I just don't know how to do it)
Let me know if there's a way to test that part.

@MelvinBot
Copy link

🎯 @thesahindia, thanks for reviewing and testing this PR! 🎉

An E/App issue has been created to issue payment here: #15407.

@mountiny
Copy link
Contributor Author

@thesahindia thats fine, at the moment here is no way to add other admins in NewDot afaik.

That is handled in Olddot and the chat should be pinned in the backend the optimistic data will be added once we add this action to front end

@thesahindia
Copy link
Member

Cool! We are good to merge this.

@mountiny mountiny merged commit 524ce78 into main Feb 23, 2023
@mountiny mountiny deleted the vit-pinAdminChannels branch February 23, 2023 11:57
@github-actions
Copy link
Contributor

Performance Comparison Report 📊

Significant Changes To Duration

There are no entries

Meaningless Changes To Duration

Show entries
Name Duration
App start TTI 711.169 ms → 740.762 ms (+29.593 ms, +4.2%)
App start runJsBundle 200.094 ms → 205.844 ms (+5.750 ms, +2.9%)
Open Search Page TTI 615.036 ms → 616.768 ms (+1.733 ms, ±0.0%)
App start nativeLaunch 19.467 ms → 20.452 ms (+0.985 ms, +5.1%)
App start regularAppStart 0.014 ms → 0.015 ms (+0.001 ms, +9.1%)
Show details
Name Duration
App start TTI Baseline
Mean: 711.169 ms
Stdev: 31.166 ms (4.4%)
Runs: 657.1801699995995 663.1765970000997 665.2902300003916 677.0980949997902 677.711796999909 679.1700189998373 680.3867990002036 687.4025039998814 690.0030410001054 691.1284870002419 693.3495140001178 699.5420430004597 702.7372709996998 704.8646999998018 705.7957149995491 706.3944070003927 712.2213880000636 714.9226110000163 715.2320400001481 721.0304939998314 724.9182620001957 725.5465810000896 727.7181759998202 735.7229369999841 738.2343979999423 741.7966229999438 748.3612590003759 748.5835990002379 760.1237570000812 771.3915499998257 779.1995449997485

Current
Mean: 740.762 ms
Stdev: 29.104 ms (3.9%)
Runs: 689.0848830007017 702.3422260005027 704.511548999697 716.0938230007887 716.6656080000103 716.7571949996054 716.7777249999344 717.2341309990734 717.7114390004426 718.354524999857 719.1955629996955 721.2177709992975 722.6163159999996 726.477811999619 727.9806730002165 729.3965099994093 733.6017829999328 738.2270369995385 747.9536569993943 750.1413950007409 754.1454349998385 754.3884800001979 759.092738000676 759.2665550000966 762.4977579992265 763.5615940000862 767.1398060005158 768.5023619998246 785.7535500004888 792.5678509995341 802.1596460007131 802.9593080002815
App start runJsBundle Baseline
Mean: 200.094 ms
Stdev: 27.776 ms (13.9%)
Runs: 164 168 168 169 174 174 174 176 176 178 180 180 181 188 189 190 197 197 206 208 210 211 214 223 224 226 227 227 237 239 251 277

Current
Mean: 205.844 ms
Stdev: 24.495 ms (11.9%)
Runs: 173 175 175 180 183 186 188 189 189 189 190 191 195 195 198 198 198 201 205 211 211 211 212 220 225 227 228 229 234 239 269 273
Open Search Page TTI Baseline
Mean: 615.036 ms
Stdev: 19.301 ms (3.1%)
Runs: 575.0109859993681 588.2987869996578 590.3786219991744 592.2536619994789 593.4177659992129 595.9924719985574 596.4337560003623 600.6308189993724 601.4315179996192 602.1050620004535 605.2728690002114 606.3385010007769 606.734170999378 607.3147790003568 610.6925050001591 613.137043999508 615.3684089994058 617.1444500004873 618.3802080005407 618.579183999449 620.1628830004483 621.2621260005981 621.7785240001976 622.940470000729 625.437947999686 627.7543540000916 628.9414059994742 633.6993409991264 636.4589029997587 637.9920249991119 651.8780119996518 656.0292969997972 656.9283450003713

Current
Mean: 616.768 ms
Stdev: 27.638 ms (4.5%)
Runs: 576.9742029998451 579.7060959991068 581.6738280002028 583.5112709999084 589.2323400005698 589.8523360006511 590.288412000984 596.4934899993241 596.5100910011679 599.6486809998751 600.5236009992659 603.8892010003328 605.1535240001976 606.059366999194 607.3220620006323 607.453409999609 607.7043460011482 610.5480150002986 613.4975589998066 617.7320560012013 619.0500489994884 619.3796800002456 631.2907310016453 631.5878509990871 631.9165449999273 633.0952160004526 641.292887000367 644.5701909996569 649.5886639989913 667.5312510002404 670.6777749992907 671.8662519995123 677.7361249998212
App start nativeLaunch Baseline
Mean: 19.467 ms
Stdev: 1.648 ms (8.5%)
Runs: 17 17 17 18 18 18 18 18 18 19 19 19 19 19 19 19 19 20 20 20 20 20 20 21 21 21 22 22 22 24

Current
Mean: 20.452 ms
Stdev: 2.284 ms (11.2%)
Runs: 18 18 18 18 18 18 18 18 19 19 19 19 20 20 20 20 20 20 21 21 21 21 21 22 22 22 23 23 25 26 26
App start regularAppStart Baseline
Mean: 0.014 ms
Stdev: 0.001 ms (5.7%)
Runs: 0.012045000679790974 0.012532999739050865 0.012655000202357769 0.012695000506937504 0.012776999734342098 0.012777000665664673 0.0129399998113513 0.0129399998113513 0.012940000742673874 0.013061000034213066 0.013143000192940235 0.013345999643206596 0.013346999883651733 0.013427999801933765 0.01355000026524067 0.013590999878942966 0.013632000423967838 0.013672000728547573 0.01375299971550703 0.01375299971550703 0.01387499924749136 0.013875000178813934 0.013875999487936497 0.013876000419259071 0.014200999401509762 0.014201000332832336 0.01448499970138073 0.014527000486850739 0.014770000241696835 0.015300000086426735 0.015339999459683895

Current
Mean: 0.015 ms
Stdev: 0.001 ms (5.4%)
Runs: 0.013182999566197395 0.013467999175190926 0.013508999720215797 0.013712000101804733 0.013753000646829605 0.013956999406218529 0.014119001105427742 0.014322999864816666 0.014322999864816666 0.0143630001693964 0.01440499909222126 0.014608001336455345 0.01464799977838993 0.014728998765349388 0.014730000868439674 0.014852000400424004 0.014852000400424004 0.014892000705003738 0.014932999387383461 0.015014000236988068 0.015217998996376991 0.015217998996376991 0.015257999300956726 0.015257999300956726 0.015298999845981598 0.015380000695586205 0.015420999377965927 0.015829000622034073 0.01590999960899353 0.015951000154018402 0.016276000067591667 0.01647999882698059

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by https://github.com/mountiny in version: 1.2.76-5 🚀

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

@kavimuru
Copy link

@mountiny We can't add admin from OldDot to workspace policy step 3: In OldDot, add another admin to the workspace. Clicking on WS policy will redirect to ND.
How to add admin to a WS?

@mountiny
Copy link
Contributor Author

@kavimuru I think this is fine, sorry, there is only one admin on workspaces for now, I think we can only add admins from support tools.

@mountiny
Copy link
Contributor Author

@kavimuru lets only QA this for when you create a workspace, the admins chat should be immediately pinned

@OSBotify
Copy link
Contributor

🚀 Deployed to production by https://github.com/yuwenmemon in version: 1.2.76-7 🚀

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

1 similar comment
@OSBotify
Copy link
Contributor

🚀 Deployed to production by https://github.com/yuwenmemon in version: 1.2.76-7 🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to production by https://github.com/yuwenmemon in version: 1.2.76-7 🚀

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

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