text:
Chat - Incorrect format is copied to clipboard after copying an edited header with bold markdown

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

Version Number: 1.4.85-0
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: N/A
Issue reported by: Applause - Internal Team
Action Performed:

Navigate to staging.new.expensify.com
Inside the compose box paste a header with bold markdown and post it: (e.g. # header)
Edit the posted comment and append some letters after a space. (e.g # header aa)
Triple click on the edited comment on step 2
Paste it inside the compose box

Expected Result:
The correct format gets pasted
Actual Result:
The header markdown is pasted on the first line and the rest of the text is pasted on a new line
Workaround:
Unknown
Platforms:

Which of our officially supported platforms is this issue occurring on?

[ ] Android: Native
[ ] Android: mWeb Chrome
[ ] iOS: Native
[ ] iOS: mWeb Safari
[x] MacOS: Chrome / Safari
[ ] MacOS: Desktop

Screenshots/Videos
Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/7f22f51b-3c2f-4b07-a12f-5ac5c0e3d68f
View all open jobs on GitHub

@garrettmknight  FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

We think that this bug might be related to #vip-vsp

Adding to live markdown per this

Waiting for SWM to work on this one.

Hi, I’m Bartosz from SWM react-native-live-markdown team. 😄
I believe this issue is something that could be handled by external contributors, especially considering that it's probably going to require changes just in E/App. WDYT @garrettmknight ?

Hi there, I can't reproduce it on my latest version. (050641a: Update version to 9.0.2-3)
# *Header* s

https://github.com/Expensify/App/assets/26862281/4bca4c64-d012-4456-acfb-822c0f354a2a

Proposal
Please re-state the problem that we are trying to solve in this issue.
We are adding \n after # in the input when header includes additional tags like _Text_ or *Text* is present in the text.
What is the root cause of that problem?
We add \n at the end of the line in replaceBlockElementWithNewLine: here
What changes do you think we should make in order to solve the problem?
I got stable and successful results changed the first "if" statement conditions to:
if (text.match(/[\n|>][>]?[\s]?$/) || index === splitText.length - 1 || text === "# ") {
  joinedText += text;
}

https://github.com/Expensify/App/assets/26862281/9d4785a0-aefa-46c3-81af-de9e3d563a6a

@rushatgabhane can you give this one a look when you get a chance?

@Amoralchik's proposal looks good https://github.com/Expensify/App/issues/43931#issuecomment-2196578436
we have unit tests on the library so it should be good 👍
🎀👀🎀

@hayata-suenaga can you give this one a look when you get a chance?

sorry I have been sick 🙇 @Amoralchik's proposal looks good to me 😄

I created the pull request.

reviewing the PR

Hi there, The changes made here are now available on staging and ready for testing, by this bump expensify-common

@joekaufmanexpensify just need you to watch this one while I'm out - I'll pick back up when I'm back if it's still going. Thanks!

Sounds good! Moving back to weekly as I see PR is in review.

Hi, the changes are in production https://github.com/Expensify/App/pull/45556#issuecomment-2249280004

@joekaufmanexpensify The PR in the App has been closed as the expensify-common update has been completed here: https://github.com/Expensify/App/pull/45556#issuecomment-2249280004. We can either proceed to testing or close it with full payment per the example here: https://github.com/Expensify/App/issues/44835#issuecomment-2248342152.
As mentioned in #44835, #45556 updated the version without following a proper process for issues tracking, and this wasn't the only issue affected. See, for example: #37357 (comment) and #44032 (comment).

Got it. Just to make sure my understanding is correct:

https://github.com/Expensify/expensify-common/pull/744 was the PR that fixed the actual bug here.
Because of the nature of the bug, the fix was brought to production via https://github.com/Expensify/App/pull/45556#issuecomment-2249280004 , which bumped the expensify-common version (alongside some other fixes).

Is that right? If so, payment would be due 7 days after the fix went to production, which would be 2024-07-30.

Awesome. TY! So payment is due tomorrow in that case. We need to pay:

@Amoralchik - $250 for PR via Upwork.
@rushatgabhane - $250 for C+ via NewDot.

@Amoralchik offer sent for $250!

@joekaufmanexpensify accepted

$250 approved for @rushatgabhane

@Amoralchik $250 sent and contract ended!

All set, thanks everyone!

authors: Amoralchik BartoszGrajdek JmillsExpensify garrettmknight hayata-suenaga joekaufmanexpensify lanitochka17 rushatgabhane


text:
Web - Integrations - Import / Export menu disappears after sync / connection for QBO / Xero

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

Version Number: 9.0.14-1
Reproducible in staging?: Y
Reproducible in production?: N
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4784244
Email or phone of affected tester (no customers): applausetester+79pronin@applause.expensifail.com
Issue reported by: Applause - Internal Team
Action Performed:

Go to https://staging.new.expensify.com/
Create a new workspace
Go to workspace settings > "More Features" > Enable the "Accounting" toggle
Go to Accounting
Connect QBO or Xero

Expected Result:
Import / Export / Advanced menu should be displayed
Actual Result:
Import / Export / Advanced menu appears for a second and then disappears
Workaround:
Unknown
Platforms:

Which of our officially supported platforms is this issue occurring on?

[ ] Android: Native
[ ] Android: mWeb Chrome
[ ] iOS: Native
[ ] iOS: mWeb Safari
[x] MacOS: Chrome / Safari
[ ] MacOS: Desktop

Screenshots/Videos
Add any screenshot/video evidence

https://github.com/user-attachments/assets/07026c51-2e1a-4e71-a24a-8bc05931217b
View all open jobs on GitHub

Dupe of https://github.com/Expensify/App/issues/46445

Closing as dupe

source: gharchive/issue

repo: Expensify/App

license: MIT

license_type: permissive

license_source: github-api


text:
Report fields and values don't get updated in Report if admin deletes them from workspace

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

**Version Number:v9.0.30-19
**Reproducible in staging?:Y
**Reproducible in production?:Y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by:
Slack conversation:
Action Performed:

Logged into NewDot as the admin of a workspace in one window and a member in another.
Go to workspace settings as admin.
Go to the report fields section.
Make sure there are list values on the workspace report fields.
As a member, open an expense's report fields.
As an admin, delete all of the values.

Expected Result:
Report should update to show that fields were updated/deleted.
Actual Result:
Nothing changes in the member report, despite fields and values having changed for the workspace.
Workaround:
Unknown.
Platforms:

Which of our officially supported platforms is this issue occurring on?

[ ] Android: Native
[ ] Android: mWeb Chrome
[ ] iOS: Native
[ ] iOS: mWeb Safari
[x] MacOS: Chrome / Safari
[ ] MacOS: Desktop

Screenshots/Videos
Add any screenshot/video evidence
https://github.com/user-attachments/assets/0acad528-e111-455e-9935-bded2149ecdf

View all open jobs on GitHub
Issue OwnerCurrent Issue Owner: @aimane-chnaif

Proposal
Please re-state the problem that we are trying to solve in this issue.
Report fields and values don't get updated in Report if admin deletes them from workspace
What is the root cause of that problem?
We are using reportFields here instead of policy fields
https://github.com/Expensify/App/blob/1b479b0602158fea8eb427fb215a14a18926c58f/src/pages/EditReportFieldPage.tsx#L54
but when report fields are updated it is the policy fields that gets updated
What changes do you think we should make in order to solve the problem?

We should use policy fields
    const reportField =  policy?.fieldList?.[fieldKey];

or
    const reportField = policy?.fieldList?.[fieldKey] ?? report?.fieldList?.[fieldKey]  ;

What alternative solutions did you explore? (Optional)

Proposal
Please re-state the problem that we are trying to solve in this issue.
Nothing changes in the member report, despite fields and values having changed for the workspace.
What is the root cause of that problem?
When we delete the value of the list report field, the fieldList of the expense report isn't updated. It will be updated if we reset the cache.
https://github.com/Expensify/App/blob/172504115675f5c6aad87fe4e96e8166b2183b14/src/pages/EditReportFieldPage.tsx#L54
https://github.com/Expensify/App/blob/172504115675f5c6aad87fe4e96e8166b2183b14/src/pages/EditReportFieldPage.tsx#L156
What changes do you think we should make in order to solve the problem?
For fieldOptions, we should get this from the data of policy then it can be up-to-date every time we change the list value of policy field list. We should not change the reportField to prioritize the policy data first because the fieldList of the report has the current value data of the report field that we set in this report then it used to display the current value that doesn't have in policy.fieldList
const policyReportField = policy?.fieldList?.[fieldKey];
fieldOptions={(policyReportField?.values ?? []).filter((_value: string, index: number) => !policyReportField?.disabledOptions[index])}

https://github.com/Expensify/App/blob/172504115675f5c6aad87fe4e96e8166b2183b14/src/pages/EditReportFieldPage.tsx#L156
OPTIONAL: If there is no value to select we can display the empty view for EditReportFieldDropdownPage as in ReportFieldsListValuesPage and we can display a button at the bottom that will redirect report field page or edit page if the current user can edit report field of the WS

What alternative solutions did you explore? (Optional)

Sorry forgot to notify Updated
But note that my last update was hours before the other proposals above

@puneetlath payment overdue

@puneetlath can u pls issue the payment here? Thx!

Hm, seems there is no upwork job.
@FitseTLT offer here: https://www.upwork.com/nx/wm/offer/104341899
@aimane-chnaif offer here: https://www.upwork.com/nx/wm/offer/104341902
@aimane-chnaif can you please complete the checklist?

@puneetlath offer accepted!

@FitseTLT has been paid. Just waiting on @aimane-chnaif.

@aimane-chnaif can you accept the offer: https://www.upwork.com/nx/wm/offer/104341902

Paid!

authors: FitseTLT carlosmiceli nkdengineer puneetlath


text:
Hybrid - Workspace - Upgrade page in category section flickers.

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

Version Number: V9. 0.74-0
Reproducible in staging?: Yes
Reproducible in production?: Yes
If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: No, reproducible on hybrid only
Issue reported by: Applause Internal Team
Action Performed:

Launch app in both mweb and android
Go to workspace settings - category
Tap on a category
Open GL code and tap back using top left button
Repeat the same step few times to note upgrade page flickers
Open payroll code and tap back using top left button
Repeat the same step few times to note upgrade page flickers

Expected Result:
Upgrade page in category section must not flicker.
Actual Result:
Upgrade page in category section flickers.
Workaround:
Unknown
Platforms:

[ ] Android: Standalone
[x] Android: HybridApp
[ ] Android: mWeb Chrome
[ ] iOS: Standalone
[ ] iOS: HybridApp
[ ] iOS: mWeb Safari
[ ] MacOS: Chrome / Safari
[ ] MacOS: Desktop

Screenshots/Videos
https://github.com/user-attachments/assets/5982a444-3a2c-462b-bb9f-16ef303656ba
View all open jobs on GitHub

getting to this one tomorrow

authors: IuliiaHerets adelekennedy


text:
mWeb (Safari) - iOS 15 - Content is hidden by address bar when keyboard is open

Action Performed:

Navigate to any conversation
Press on input to open the keyboard

Expected Result:
Input should slide up above the address bar
Actual Result:
Address bar is partially covering the input
Workaround:
Go to Settings - Safari. Change Tabs to "Single tab", which moves address bar to the top.
Platform:
Where is this issue occurring?

Mobile Web

iOS 15 to be precise
**Version Number: 1.1.20-2
**Reproducible in staging?: Yes
**Reproducible in production?: Yes
**Notes/Photos/Videos:
https://user-images.githubusercontent.com/9059945/145996496-4583cc3d-91b1-46a4-bcd5-b64482a23b8e.mp4
https://user-images.githubusercontent.com/9059945/145996515-27126247-95be-460d-ae82-991878666044.mp4
Expensify/Expensify Issue URL:
**Issue reported by: eVoloshchak
Slack conversation:
View all open jobs on GitHub

Triggered auto assignment to @kevinksullivan (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

Triggered auto assignment to @tylerkaraszewski (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

Likely external, but going to tap in engineering before going with the export.

External seems fine.

A similar issue is being fixed here. It's a duplicate.
@eVoloshchak We prefer issues to be reported on our Slack channel. You can join that and report there. Next time issues are created like this be closed without any reason.

@eVoloshchak We prefer issues to be reported on our Slack channel. You can join that and report there. Next time issues are created like this be closed without any reason.

Yeah, sorry, my bad, I should of checked contributing guidelines before opening the issue.

closing as dupe

authors: MelvinBot eVoloshchak jboniface kevinksullivan parasharrajat tylerkaraszewski


text:
Windows - Cursor move to next line if you press Enter

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

Action Performed:

Go to https://staging.new.expensify.com and log in with any account
Go to any conversation
Start typing the message and press Enter

Expected Result:
Message should be sent after press Enter
Actual Result:
Cursor move to next line if you press Enter
Workaround:
Unknown
Platform:

Where is this issue occurring?

Web/ Windows

Version Number: 1.1.38.1
Reproducible in staging?: Yes
Reproducible in production?: Yes
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
https://user-images.githubusercontent.com/93399543/153694521-6ad03a2e-1604-4c7d-a092-e3cb970b44e9.mp4
https://user-images.githubusercontent.com/93399543/153694529-8d8c4f90-28fd-4365-9aa1-99f04f4de4f9.mp4
Expensify/Expensify Issue URL:
Issue reported by: Applause
Slack conversation:
View all open jobs on GitHub

Triggered auto assignment to @chiragsalian (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

Hmm i'm not able to reproduce this on staging or production. Are you still able to consistently reproduce this and are you sure the shift key was not held?

Friendly bump, any update to my question above @kbecciv? 🙂

@chiragsalian Yes, I am consistently reproduce this issue, no shift key was held for this action.

Hmm weird, I cannot reproduce. Anyway no worries i'll open it up to contributors that can test and figure out the problem.

Triggered auto assignment to @michaelhaxhiu (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

I also cannot reproduce this bug, and that's typically a mandetory step for me as a contributor manager before externally assigning this job.
In order to proceed, I think we need more context on how to reproduce this issue. I followed steps under "Action Performed" and whenever I hit enter it always sends the message exactly as expected.  @kbecciv sorry to ping you again but any chance you can try to add more context here?

@michaelhaxhiu I understand that you may have an issue reproducing the issue. It is not repro on MacOS. We have multiple users who are facing this and it seems to be Device specific.
E.g every user with ASUS laptop is able to see it.

@mvtglobally In any case Do those laptops have a touch screen?

@parasharrajat i can ask others, but my laptop has a touchscreen where I can repro the issue

Gotcha. Ok, I can see which PR caused it and why?
This is the culprit code https://github.com/Expensify/App/blob/6ef2e519d1ecf5b0e6db3f5498e24c7e50e9f21b/src/pages/home/report/ReportActionCompose.js#L343.
Although, the last commit is mine :smile_cat:. But this change was originally added https://github.com/Expensify/App/pull/7355 .
In Short, if a device has a touch screen Enter to submit shortcut is disabled. But I see that it can be irritating for touch screen laptop users who are not really using the touchscreen.

@michaelhaxhiu, did you post this job in upwork? If not please do. Looks like it's specific to touchscreen devices.
@parasharrajat, thanks for the find. I'm not sure if the last statement is a solution. Either way, can you explain where you'll be adding the code and what would it be for clarity. Additionally, I was curious if this is something you can test or not? Worst case we can still push out the fix and ask QA if it's been resolved.

I haven't proposed anything yet https://github.com/Expensify/App/issues/7715#issuecomment-1047280722 is an explanation of the root cause.
Propose.
I don't see any solution that allows us to detect that the user is using an external keyboard so we will have to detect that the user is on desktop.
Possible options.
2. dimensions check.
3. UserAgent check.
4. feature detection.
I will try to find a bulletproof solution for it.

Cool, thanks for clarifying. I'll bump this down to weekly since i don't feel like its a daily. If anyone disagrees feel free to bump it up.

@michaelhaxhiu, did you post this job in upwork? If not please do. Looks like it's specific to touchscreen devices.

done
https://www.upwork.com/jobs/~01e9d1a86d39175b4d

Triggered auto assignment to Contributor-plus team member for initial proposal review - @parasharrajat (Exported)

Current assignee @chiragsalian is eligible for the Exported assigner, not assigning anyone new.

We could listen onkeyPress from TextView and inside the listener we can check if key pressed is Enter, and if yes, we will call the sendMessage() function

This is still open to proposals. I will try to get a solution if I have enough time to dig in.

Price doubled to $500.

Proposal
There is a solution that we can detect that touchable feature in window.
Ref: https://hacks.mozilla.org/2013/04/detecting-touch-its-the-why-not-the-how/

Add a condition that detects touch device or not.

const isTouchDevice = () => {
    return (('ontouchstart' in window) || (navigator.maxTouchPoints > 0) || (navigator.msMaxTouchPoints > 0));
}

Add more conditions in this line
https://github.com/Expensify/App/blob/6ef2e519d1ecf5b0e6db3f5498e24c7e50e9f21b/src/pages/home/report/ReportActionCompose.js#L343

if (!e || (VirtualKeyboard.shouldAssumeIsOpen() && !isTouchDevice())) {
   return;
}

Know issue that we could face an issue with the mobile browser which is ontouchstart also exist. So I would add functionality to detect it is mobile browser or not by userAgent.

function detectMobible() {
    const toMatch = [
        /Android/i,
        /webOS/i,
        /iPhone/i,
        /iPad/i,
        /iPod/i,
        /BlackBerry/i,
        /Windows Phone/i
    ];

    return toMatch.some((toMatchItem) => {
        return navigator.userAgent.match(toMatchItem);
    });
}

Demo
Before
https://user-images.githubusercontent.com/5449119/158053252-78cfbb90-8422-446a-a97a-8a850a3b8d69.mp4
After
https://user-images.githubusercontent.com/5449119/158053254-f709791f-0244-4c1d-9fb9-1c5c812679b4.mp4

I do not understand this proposal. VirtualKeyboard.shouldAssumeIsOpen() fallbacks to touch devices to assume if the virtual keyboard is open and you are adding another condition that reverses that.
On the other hand, userAgent matching is not a bulletproof technique. Let's use feature detection instead.

It already does that. Let's work on the other request first. Thanks.

Doubling price again, as we haven't seen a solution that solves the problem yet.

Doubling price again.

Doubling price again.

PROPOSAL
Changing:
https://github.com/Expensify/App/blob/fd0f83a2e557c9f6d1adcf5be6feb761cd9f40b4/src/libs/VirtualKeyboard/index.js#L16-L26
by:
function shouldAssumeIsOpen() {
    return isOpen();
}

Won't give us the expected behavior?
I didn't get why seeing user has a touchable device will assume kayboard is open.

@parasharrajat I think it won't work when we don't have the VirtualKeyboard API(it will return us null).
We currently use this logic:
 * As of January 2022, the VirtualKeyboard web API is not available in all browsers yet
 * If it is unavailable, we default to assuming that the virtual keyboard is open on touch-enabled devices.
 * See https://github.com/Expensify/App/issues/6767 for additional context.

But now we know assuming that the virtual keyboard is open on touch-enabled devices won't give us the expected behavior when we have a desktop working with touch-enabled.
There's no way we can know if user's device is either desktop or mobile unless using userAgent to check it:
I know using userAgent isn't the optimal solution but I think its our only alternative.
So in my updated proposal I think we should:
PROPOSAL
Create a function which will tell us if user isMobile only when we don't have the VirtualKeyboard API(instead of looking if device is touch-enabled):
function isMobile(){
     /Android|webOS|iPhone|iPad|iPod|BlackBerry|BB|PlayBook|IEMobile|Windows Phone|Silk|Opera Mini/i.test(navigator.userAgent))
}

Then we would have an updated shouldAssumeIsOpen() as follows:
function shouldAssumeIsOpen() {
    const isOpened = isOpen();
    return _.isNull(isOpened) ? isMobile() : isOpened;
}

This way, when we don't have the VirtualKeyboard API:
When isMobile = true we will return shouldAssumeIsOpen true
When isMobile = false we will return shouldAssumeIsOpent false
and it will give us the expected behavior.
*Also we will use this new function(isMobile) in other parts of the project when we need it like in this case we don't have others options.

Can we use Feature detection instead of User_agent check or dimensions check? Out of these, which one do you think is best and why?

Dimensions check - I think we can't use dimensions check because we are dealing with a big amount of possible screen sizes and in my opinion it doesn't make sense to use thoses values in order to know if the device uses a virtual keyboard.
We could have desktop devices with lower width than some tablets or ipads(which will use virtual keyboard) for example.

UserAgent check - That's the best solution given our cenario(when we can't use virtual keyboard check) in my opinion. We can specify the target devices and it will run as it should.

Feature detection - I think we already do feature detection everytime we can in this part of code, doesn't we? https://github.com/Expensify/App/blob/fd0f83a2e557c9f6d1adcf5be6feb761cd9f40b4/src/libs/VirtualKeyboard/index.js#L4-L14
When we check for the virtualKeyboard API we are discarding having to use userAgent check

https://developer.mozilla.org/en-US/docs/Learn/Tools_and_testing/Cross_browser_testing/Feature_detection says:
Feature detection involves working out whether a browser supports a certain block of code, and running different code depending on whether it does (or doesn't)
We are checking virtualKeyboard when the browser supports it, and using this new isMobile logic for when it doesn't.

I meant feature detection for detecting if a device is a desktop with an external keyboard.

Dimensions check

This was/is a testing method on a few CSS frameworks like bootstrap. But I feel like, it is going to be buggy.
At last, until we break anything for users that are using isOpen api, I am good with userAgent check. Could you please tell me the places where this is used and how will user solution worth it without breaking those?

I meant feature detection for detecting if a device is a desktop with an external keyboard.
Dimensions check
This was/is a testing method on a few CSS frameworks like bootstrap. But I feel like, it is going to be buggy.

As far as I could find, people count time and make assumptions based on time between KeyDown and KeyUp  to know if the keyboard is either real or virtual:
https://stackoverflow.com/questions/18880236/how-do-i-detect-hardware-keyboard-presence-with-javascript
I think it's hacky and more a workaround than achieving the desired solution and won't work everytime.
I still strongly think we should go with userAgent check.
So in my updated proposal:
PROPOSAL
in my solution we are going to create new folder/file within https://github.com/Expensify/App/tree/main/src/libs/
isMobile/index.js
with the content:
/**
 * Allows us to identify whether the platform is mobile.
 *
 * https://developer.mozilla.org/en-US/docs/Web/HTTP/Browser_detection_using_the_user_agent
 *
 * @returns {Boolean}
 */
 function isMobile(){
        return /Android|webOS|iPhone|iPad|iPod|BlackBerry|BB|PlayBook|IEMobile|Windows Phone|Silk|Opera Mini/i.test(navigator.userAgent);
}

export default isMobile;

And use it here instead of use canUseTouchScreen() https://github.com/Expensify/App/blob/fd0f83a2e557c9f6d1adcf5be6feb761cd9f40b4/src/libs/VirtualKeyboard/index.js#L16-L26
After changes:
/**
 * As of January 2022, the VirtualKeyboard web API is not available in all browsers yet
 * If it is unavailable, we default to assuming that the virtual keyboard is open on mobile devices.
 * See https://github.com/Expensify/App/issues/7715 for additional context.
 *
 * @returns {Boolean}
 */
function shouldAssumeIsOpen() {
    const isOpened = isOpen();
    return _.isNull(isOpened) ? isMobile() : isOpened;
}

At last, until we break anything for users that are using isOpen api, I am good with userAgent check. Could you please tell me the places where this is used and how will your solution work without breaking those?

What do you think about this?

What do you think about this?

Sorry I didn't get what you want...
If you mean where we will use it:
We will use it here:
https://github.com/Expensify/App/blob/6ef2e519d1ecf5b0e6db3f5498e24c7e50e9f21b/src/pages/home/report/ReportActionCompose.js#L342-L344
Instead of use canUseTouchScreen() and assume when canUseTouchScreen = true the device is mobile we will use the userAgent check to tell us when we are on mobile devices

ok. I think this mobile userAgent check is best suited for shouldAssumeIsOpen instead of canUseTouchScreen. IMO, isMobile should be moved to getBrowser lib.
Rest, I am fine with @mateusbra's approach to this problem https://github.com/Expensify/App/issues/7715#issuecomment-1097194870. I was waiting on a proposal with feature detection but there is no such API atm. And @mateusbra's reason for dimensions check seems valid.
cc: @chiragsalian
:ribbon: :eyes: :ribbon:  C+ reviewed

Proposal LGTM as well. Feel free to create the PR @mateusbra.

I submitted a proposal on Upwork, gonna rise a PR soon!

Hired - let's get this party started.

Issue not reproducible during KI retests. (First week)

Issue not reproducible during KI retests. (Second week)

PR deployed on PROD 4 days back.

Just a quick bud check -- so does that mean payment is due on Friday for @mateusbra, assuming no regressions between now & then.
Weird the GH title didn't update to add that 🤔

Yeah, Please update the title. It must be due to the wrong link format used on PR. Please update that as well if possible.

@parasharrajat can you apply for this job - https://www.upwork.com/jobs/~0112030d1e6d50fa17 for the C+ work

@mateusbra has been paid.

Issue not reproducible during KI retests. (Third week)

Ping for

@michaelhaxhiu

Waiting for you to accept job offer @parasharrajat 👍

Paid.

authors: MelvinBot chiragsalian kbecciv lokesh-kedia mateusbra michaelhaxhiu mvtglobally parasharrajat phivh


text:
🍒 Cherry pick PR #10062 to staging 🍒

🍒 Cherry pick https://github.com/Expensify/App/pull/10062 to staging 🍒

🚀 Cherry-picked to staging by @OSBotify in version: 1.1.85-8 🚀

platform
result

🤖 android 🤖
success ✅

🖥 desktop 🖥
success ✅

🍎 iOS 🍎
success ✅

🕸 web 🕸
success ✅

@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes.

🚀 Deployed to production by @luacmartins in version: 1.1.85-8 🚀

platform
result

🤖 android 🤖
success ✅

🖥 desktop 🖥
success ✅

🍎 iOS 🍎
success ✅

🕸 web 🕸
success ✅

source: gharchive/pull-request


text:
Fix image file size when uploaded from ios

Details

upgrade the version of react-native-image-picker
add a new request handler to deal the selected image

Fixed Issues

$ https://github.com/Expensify/App/issues/9424
Tests

Launch the app
Go to a chat and upload an image from a ios device.
Check the image size on device.
Now open the chat on recipient account in a web browser.
Open and download the received image.
Check the image file size.

[x] Verify that no errors appear in the JS console

PR Review Checklist

Contributor (PR Author) Checklist

[x] I linked the correct issue in the ### Fixed Issues section above
[x] I wrote clear testing steps that cover the changes made in this PR

[x] I added steps for local testing in the Tests section
[x] 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 included screenshots or videos for tests on all platforms
[ ] I ran the tests on all platforms & verified they passed on:

[x] iOS / native
[ ] Android / native
[ ] iOS / Safari
[ ] Android / Chrome
[ ] MacOS / Chrome
[ ] MacOS / Desktop

[x] 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)
[x] I followed proper code patterns (see Reviewing the code)

[x] 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)
[x] I verified that comments were added to code that is not self explanatory
[x] 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.
[x] I verified any copy / text shown in the product was added in all src/languages/* files
[x] I verified any copy / text that was added to the app is correct English and approved by marketing by tagging the marketing team on the original GH to get the correct copy.
[x] 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.
[x] I verified the JSDocs style guidelines (in STYLE.md) were followed

[x] If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
[x] I followed the guidelines as stated in the Review Guidelines
[x] 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)
[x] I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
[x] 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
[x] If a new component is created I verified that:

[x] A similar component doesn't exist in the codebase
[x] All props are defined accurately and each prop has a /** comment above it */
[x] Any functional components have the displayName property
[x] The file is named correctly
[x] The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
[x] The only data being stored in the state is data necessary for rendering and nothing else
[x] 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)
[x] 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)
[x] All JSX used for rendering exists in the render method
[x] 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

[x] If a new CSS style is added I verified that:

[x] A similar style doesn't already exist
[x] The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)

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

PR Reviewer Checklist
The Contributor+ will copy/paste it into a new comment and complete it after the author checklist is completed

[ ] 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 verified tests pass on all platforms & I tested again on:

[ ] iOS / native
[ ] Android / native
[ ] iOS / Safari
[ ] Android / Chrome
[ ] MacOS / Chrome
[ ] 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 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 was added in all src/languages/* files
[ ] I verified any copy / text that was added to the app is correct English and approved by marketing by tagging the marketing team 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 */
[ ] Any functional components have the displayName property
[ ] 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 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.
[ ] I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

QA Steps

Launch the app
Go to a chat and upload an image from a ios device.
Check the image size on device.
Now open the chat on recipient account in a web browser.
Open and download the received image.
Check the image file size.

[x] Verify that no errors appear in the JS console

Screenshots

Web

Mobile Web

Desktop

iOS

https://youtu.be/oQQwplchSw8
Android

Hi, I thought it was made clear in the proposal 😂

react-native use RCTImageLoader#sendRequest by default to convert the image uri to binary data ( this image uri is returned by the react-native-image-picker ), and I also mentioned it in this issue
So we need to add a new handler to replace it.
Additionally, this new handler won't affect normal image loading

@mountiny @Santhosh-Sellavel,  I'm going to add a short comment. Is this ok ? 🙂
diff --git a/ios/NewExpensify/OriginImageRequestHandler.mm b/ios/NewExpensify/OriginImageRequestHandler.mm
index d6dc968f9..346b43a9e 100644
--- a/ios/NewExpensify/OriginImageRequestHandler.mm
+++ b/ios/NewExpensify/OriginImageRequestHandler.mm
@@ -4,6 +4,8 @@
 //
 //  Created by ntdiary on 2022/8/8.
 //
+//  Use this handler to upload images returned by react-native-image-picker instead of the default RCTImageLoader.mm
+//  See https://github.com/facebook/react-native/issues/33760#issuecomment-1196562161

 #import <Foundation/Foundation.h>
 #import <ReactCommon/RCTTurboModule.h>

Agree its useful

@mountiny  I can try it, and there is something I need to explain 🙂
there are also comments in its protocol file (RCTURLRequestHandler.h)
These two files are copies of RCTFileRequestHandler in react-native, the only difference is that I changed its canHandleRequest condition and handlerPriority.
They are automatically loaded by by RCTNetworking.mm and are only used  in react-native.
(RCTNetworking.mm uses RCTImageLoader to parse the image by default, like the image below)

How about this revision ?

@ntdiary
Can you update the screenshot recording to ensure, everything works as expected?
Please fill the author checklist

@mountiny Can you initiate work flows to perform checks

@Santhosh-Sellavel This issue and revision only occurs on ios platform.
I'm not so sure, should I fill in all items in the author checklist and update all platform screenshots?
or only related to this revision and ios platform?

I swear to god I pressed that button!
I think it would be nice to test this and show video of android working well at least and check the checkboxes in the list too. But we can move ahead even without it this time since it touches ios predominantly (however updating the package could theoretically cause problems to android as well)
@ntdiary @Santhosh-Sellavel Great job guys! I hope @ntdiary you will pick up more issues in the repo, it was good to work with you!

@ntdiary
You checked this

-[x] I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

@Santhosh-Sellavel uh, sorry, I canceled it first. 😅
i'm still trying to run the App on Android,
but it is running very slow, no matter if i build it from main branch or this branch
I'm checking what's going on.

@Santhosh-Sellavel Any chance you could help out with testing on android? I guess once confirmed all works well we can approve and merge 🙌

PR 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 verified tests pass on all platforms & I tested again on:

[ ] iOS / native
[ ] Android / native
[ ] iOS / Safari
[ ] Android / Chrome
[ ] MacOS / Chrome
[ ] 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 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 was added in all src/languages/* files
[ ] I verified any copy / text that was added to the app is correct English and approved by marketing by tagging the marketing team 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 */
[ ] Any functional components have the displayName property
[ ] 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 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.
[ ] I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Hi, I have tested it on android and updated the checklist, although it runs slowly.
And I found this issue, it seems that everyone has encountered this problem in DEV env.

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

👋 Just making a note that this introduced a regression with the Take Photo attachment functionality as noted here. We're adding a regression test for our QA suite, but let's be careful to ensure we're test both types of image uploads in the future (from file / from taking a photo). Thanks!

authors: OSBotify Santhosh-Sellavel mountiny ntdiary trjExpensify


text:
Implement Invite message page

@thienlnam @mollfpr
Details

Implementing a new Workspace members invite message page to fix the overlaying of message in the invite members page.
Fixed Issues

$ https://github.com/Expensify/App/issues/15083
PROPOSAL: https://github.com/Expensify/App/issues/15083#issuecomment-1441221893
Tests

Go Workspace -> Manage Members -> Invite Members
Choose members to invite -> Next
New invite message page to be shown with choosen members and invite message
Save should add the members to workspace

[x] Verify that no errors appear in the JS console

Offline tests

Go Workspace -> Manage Members -> Invite Members
Choose members to invite -> Next
New invite message page to be shown with choosen members and invite message
Save should add the members to workspace

QA Steps

Go Workspace -> Manage Members -> Invite Members
Choose members to invite -> Next
New invite message page to be shown with choosen members and invite message
Save should add the members to workspace

[x] Verify that no errors appear in the JS console

PR Author Checklist

[x] I linked the correct issue in the ### Fixed Issues section above
[x] I wrote clear testing steps that cover the changes made in this PR

[x] I added steps for local testing in the Tests section
[x] I added steps for the expected offline behavior in the Offline steps section
[x] I added steps for Staging and/or Production testing in the QA steps section
[x] I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
[x] 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)
[x] 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).

[x] I included screenshots or videos for tests on all platforms
[x] I ran the tests on all platforms & verified they passed on:

[x] Android / native
[x] Android / Chrome
[x] iOS / native
[x] iOS / Safari
[x] MacOS / Chrome / Safari
[x] MacOS / Desktop

[x] 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)
[x] I followed proper code patterns (see Reviewing the code)

[x] 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)
[x] I verified that comments were added to code that is not self explanatory
[x] 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.
[x] I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method

[x] 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:

[x] I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
[x] 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.
[x] 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.
[x] I verified the JSDocs style guidelines (in STYLE.md) were followed

[x] If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
[x] I followed the guidelines as stated in the Review Guidelines
[x] 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)
[x] I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
[x] 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
[x] I verified that if a function's arguments changed that all usages have also been updated correctly
[x] If a new component is created I verified that:

[x] A similar component doesn't exist in the codebase
[x] All props are defined accurately and each prop has a /** comment above it */
[x] The file is named correctly
[x] The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
[x] The only data being stored in the state is data necessary for rendering and nothing else
[x] 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)
[x] 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)
[x] All JSX used for rendering exists in the render method
[x] 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

[x] If any new file was added I verified that:

[x] 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

[x] If a new CSS style is added I verified that:

[x] A similar style doesn't already exist
[x] The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)

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

Screenshots/Videos

Web
https://user-images.githubusercontent.com/46707890/223535362-04f5a0d4-9551-4417-a290-22c0845794f5.mp4

Mobile Web - Chrome
https://user-images.githubusercontent.com/46707890/223535383-2bc0198e-5bc1-4989-8ffa-992241321968.mp4

Mobile Web - Safari
https://user-images.githubusercontent.com/46707890/223535415-10c0243e-4f17-49e3-9a95-9951be610df9.mp4

Desktop
https://user-images.githubusercontent.com/46707890/223535467-fd3ed23a-d1f6-458a-9963-727617d94144.mp4

iOS
https://user-images.githubusercontent.com/46707890/223535494-e0a973d3-b04b-4a51-964c-22079f17e552.mp4

Android
https://user-images.githubusercontent.com/46707890/223535526-558af778-0eb8-41bd-9a16-886ea0ef4580.mp4

Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers!

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

@abdulrahuman5196 Could you resolve the conflict? Thanks!

@abdulrahuman5196 Could you resolve the conflict? Thanks!

Resolved the conflicts

@abdulrahuman5196 We don't have the +1 avatar if more than 4 members have been added to the list.
https://user-images.githubusercontent.com/25520267/224711393-8d038634-6b56-4f6e-a42e-7573700a369b.mov

The avatar is also not showing on the iOS
https://user-images.githubusercontent.com/25520267/224719291-c92b3799-8335-4b86-b000-15ed783a0e24.mp4

@mollfpr  Could you kindly check if you are using the latest commit from the PR. Its working fine for me. I have been merging with main to resolve conflicts.

@abdulrahuman5196 Thanks; I'll test it again with the latest commit. I might be missing something before.

@abdulrahuman5196 Could you update the PR to make the WorkspaceInvitePage and WorkspaceInviteMessagePage can work with Form.js under the hood? Also, check if the welcomeNote update when locale change is working too.

@luacmartins Hey, I need your help here! We try using Form.js for the new page WorkspaceInviteMessagePage to send the message to the new member. However, in this PR, the input value is not handled by Form.js, so we should let Form.js take it. Here's the page I'm talking about.
Second, this PR is significant in solving the original issue. Could we use the original implementation and handle the refactor to use Form.js on other issues? cc @thienlnam

@luacmartins Hey, I need your help here! We try using Form.js for the new page WorkspaceInviteMessagePage to send the message to the new member. However, in this PR, the input value is not handled by Form.js, so we should let Form.js take it. Here's the page I'm talking about.
Second, this PR is significant in solving the original issue. Could we use the original implementation and handle the refactor to use Form.js on other issues? cc @thienlnam

@mollfpr  I agree here on the callouts. I made the new implementation with the latest features available, but I intentionally made less refractor on existing code logic unrelated to the problem like 'default message and its translation logic' to avoid any unwanted regressions. But if we want to refractor existing logics like refractoring WorkspaceInvitePage to use Form.js and other refractors we can still do, but it might not add value to solution we are trying to solve.
So a confirm here would be good before I can start making those changes, if we still want to refractor existing code?

Thank you @abdulrahuman5196, for the summary. I don't have any concerns with creating a new issue for the form refactor. @thienlnam @luacmartins I'll let you guys decide it.

@luacmartins @thienlnam Could you confirm 2 points of @abdulrahuman5196 summary here so we can move forward with the PR? Thank you!

I agree with the summary, this PR is already getting pretty large

@mollfpr @thienlnam @luacmartins
I have addressed all the comments mentioned and pushed the required changes. Kindly review and let me know if more is required.
Note: Kindly delete local checkout and make a new checkout to test my changes as I had to rebase this PR branch to remove the unsigned commits

@mollfpr  Good catch on the regression and the order change in the workspace list.
I have fixed the same and also updated the styles as per the recommendations. Kindly check and let me know if anything else is required from my end.

Hi, Just noting that we had this bug on mWeb on old Invite members page https://github.com/Expensify/App/issues/10906. I would say we test this flow to make sure it works.

@mollfpr  Any updates on the review? I think I have closed out on all existing comments. Kindly let me know if anything else needs to be done at my end

@abdulrahuman5196 Sorry for the delay! It seems looks good now, I’ll finish the review today!

Here are the regression I found @abdulrahuman5196

The last members draft should be clear after submit

https://user-images.githubusercontent.com/25520267/229288608-ffbabd6a-e964-47ea-a6ba-ec343f765da4.mov

I can submit an invite to the workspace with no selected user. Current staging is disabling the button when no user is selected.  After the submission, there are errors user in the list?

https://user-images.githubusercontent.com/25520267/229288646-0264c9a1-fbc0-4ff5-b530-ec32254d5118.mov

Privacy link is not visible when the keyboard opens. Based on the design, the link is visible.

https://user-images.githubusercontent.com/25520267/229288777-66bf8263-cc93-4431-a19b-9e1974176eb0.mp4

@ abdulrahuman5196 That's all for me. Everything else is seems working fine.

@mollfpr
for 1) I have fixed it.
for 2) Currently there is no disable option in form.js submit component - https://github.com/Expensify/App/blob/main/src/components/Form.js#L326. I assume it has to do with us showing error message when user presses submit. But our usecase seems to be a special, so I can either expose disabled props from form.js and populate it via our page so that the button is disabled or show errors message via validate but the user won't be able to take any action by pressing fix the errors. So what should be the expected behaviour here
for 3) I think we have to move the footer component into the FormAlertWrapper component from form.js(current implementation) https://github.com/Expensify/App/blob/main/src/components/FormAlertWrapper.js#L53. Wanted to confirm if the approach is ok as its a core component

so I can either expose disabled props from form.js and populate it via our page so that the button is disabled or show errors message via validate but the user won't be able to take any action by pressing fix the errors.

I'm unsure about this https://github.com/Expensify/App/blob/main/contributingGuides/FORMS.md#submit-button-disabling. But that is the straightforward way.
@luacmartins Is this okay to expose the disabled option for the submit button in Form.js?

I think we have to move the footer component into the FormAlertWrapper component from form.js(current implementation) https://github.com/Expensify/App/blob/main/src/components/FormAlertWrapper.js#L53. Wanted to confirm if the approach is ok as its a core component

I think that's okay. What do you think guys? @thienlnam @luacmartins

@luacmartins

The way we should handle this is by showing an error on validate. Can we focus the search text input when the user presses fix the errors?

I am not sure if it would be meaningful. The user can't take any action even if we focus on the text input, any input doesn't make the submit available since there is no members selected. Should we show some specific errors in this case? Some design input would be great.

I think the Privacy link looks a bit odd below the Invite button. Has this design been approved by our design team?

yes. It was the initial design mocks provided - https://github.com/Expensify/App/issues/15083#issuecomment-1441221893

We can use the DotIndicatorMessage component to show the message.

Or not allowing the user to go to WorkspaceInviteMessagePage if no members are selected by redirecting them back to WorkspaceInvitePage.

@luacmartins gentle ping on the above.

Gently bump @luacmartins for the above suggestion.

Carlos has been out of office, but should be back this week just FYI

@abdulrahuman5196 @mollfpr apologies for the delay, I'm back from ooo today!

No worries!

IMO selecting the text input still makes sense since it prompts the user to search for a new member and select them. I think in this case we could also display the error message below the text input.

Sounds good to me 👍

@abdulrahuman5196 Do you have a question regarding this comment?

No questions on this. I will close it over the weekend.

Any updates here?

I will try to close the new changes today. Caught up in personal work.

@luacmartins @mollfpr  @thienlnam
I fixed the requirements.

Privacy link to show below
Show error on no invite members present

Currently I am showing dummy text as error. Can we kindly get the proper error string and its translation for this case?

@luacmartins Unit test is only failing because translated string is not added for the new error as I have called out here - https://github.com/Expensify/App/pull/15672#issuecomment-1514443499
I assume expensify team members provide the string and translation. So IMO, I don't think we should block the review for this. I will add the translation once available and UT should be fixed after that.

@abdulrahuman5196 FINAL COPY:
Please select at least one member to invite
Por favor, selecciona al menos un miembro a invitar

@thienlnam  Thank you for providing the translations.
I have added the translations and fixed the conflicts by merging from main and all jobs are green now.
Let me know if anything else is required from my end.
@luacmartins  @mollfpr

I'll finish my review within an hour, but we already have what we need now 👍

@mollfpr I had merged from main now and not seeing the regression. And I have also addressed your comments

Reviewer Checklist

[x] I have verified the author checklist is complete (all boxes are checked off).
[x] 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 grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is 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 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 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

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

@mollfpr  Let me know if anything is blocking your testing

@mollfpr Added FullPageNotFoundView as well. You had already approved, could you test and approve again?
Other reviewers already approved. So Once @mollfpr  approves again, @thienlnam @luacmartins  Could you kindly take it forward.

Reviewer Checklist

[x] I have verified the author checklist is complete (all boxes are checked off).
[x] I verified the correct issue is linked in the ### Fixed Issues section above
[x] I verified testing steps are clear and they cover the changes made in this PR

[x] I verified the steps for local testing are in the Tests section
[x] I verified the steps for Staging and/or Production testing are in the QA steps section
[x] 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)
[x] 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)

[x] I checked that screenshots or videos are included for tests on all platforms
[x] I included screenshots or videos for tests on all platforms
[x] I verified tests pass on all platforms & I tested again on:

[x] Android / native
[x] Android / Chrome
[x] iOS / native
[x] iOS / Safari
[x] MacOS / Chrome / Safari
[x] MacOS / Desktop

[x] 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
[x] I verified proper code patterns were followed (see Reviewing the code)

[x] 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).
[x] I verified that comments were added to code that is not self explanatory
[x] 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.
[x] I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
[x] I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
[x] 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 approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
[x] 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.
[x] I verified the JSDocs style guidelines (in STYLE.md) were followed

[x] If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
[x] I verified that this PR follows the guidelines as stated in the Review Guidelines
[x] 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)
[x] I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
[x] 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
[x] If a new component is created I verified that:

[x] A similar component doesn't exist in the codebase
[x] All props are defined accurately and each prop has a /** comment above it */
[x] The file is named correctly
[x] The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
[x] The only data being stored in the state is data necessary for rendering and nothing else
[x] 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)
[x] 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)
[x] All JSX used for rendering exists in the render method
[x] 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

[x] If any new file was added I verified that:

[x] 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

[x] If a new CSS style is added I verified that:

[x] A similar style doesn't already exist
[x] The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)

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

Screenshots/Videos

Web

https://user-images.githubusercontent.com/25520267/234632445-f396ad6d-802c-4e04-8d4a-cc38820fdc91.mp4

Mobile Web - Chrome

https://user-images.githubusercontent.com/25520267/234632553-7b2af275-4ab3-474f-bf39-579ca0a16638.mp4

Mobile Web - Safari

https://user-images.githubusercontent.com/25520267/234632587-4580f126-4082-44fe-b6aa-fe02629b3b2c.mp4

Desktop

https://user-images.githubusercontent.com/25520267/234632512-155e4dcf-d714-4b92-a0f5-bc483a471941.mp4

iOS

https://user-images.githubusercontent.com/25520267/234632471-14bcdc1f-acbc-4d62-98f1-9f9f3d0b96ce.mp4

Android

https://user-images.githubusercontent.com/25520267/234632612-8c0ab2c8-fe13-487f-a1bc-65699c1e97f2.mp4

@abdulrahuman5196 Could you fix the conflict? Thanks!

@mollfpr  Made a merge from main. Kindly do the needful

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

@luacmartins  We got to do a merge conflict resolution yesterday. And this particular change which seems to have merged at Jan 16 by you showed as a merge conflict yesterday. Not sure why it is showing all of a sudden.
https://github.com/Expensify/App/blob/a7863493a03970cdc0687621ec40dc75aea99477/src/pages/workspace/WorkspaceInvitePage.js#L249-L252
But with this change it is causing issue where if we move to messagepage and come back 'Next' button is disabled in invitepage(Since the state is not updated on navigation back).
To fix this we can remove you change, I don't think we need that fix to block next button from multiple presses since it only goes to the new invite message page.
I can bring up a PR to remove your change. Kindly let me know your thoughts

@abdulrahuman5196 I'm confused. Do you have a link to the PR you are referring to? How can a change merged on Jan 16 show as a merge conflict now?

@luacmartins  this is the PR https://github.com/Expensify/App/pull/14298

@luacmartins  Can I proceed with removing the change here https://github.com/Expensify/App/pull/14298? I think we don't need it anymore since we are routing to new page.

🚀 Deployed to staging by https://github.com/thienlnam in version: 1.3.9-12 🚀

platform
result

🤖 android 🤖
success ✅

🖥 desktop 🖥
success ✅

🍎 iOS 🍎
success ✅

🕸 web 🕸
success ✅

There is regression from this PR check here

@abdulrahuman5196 yes, we can remove that.

This broke changes we introduce on https://github.com/Expensify/App/pull/16764/ to optimistically add the users to Policy Expense Chats.
Currently the functionality is broken on staging only but not prod luckily but this should be considered a blocker IMO.

Who is fixing it? If it broke something can we revert it? What's the plan?

So it looks like there's maybe two related issues with this PR:
Issue 1
Reported here
BUG ‘Next’ button is disabled on back navigation even if there is a selected user; as a result user cannot continue to invite the selected user after navigating back
Action Performed:

Click on ‘Profile’ avatar
Click on ‘Workspaces’ link
Open an existing workspace
Click on ‘Manage members’ link
Click on ‘Invite’ button
Enter email address > select user > click on ‘Next’ button
Click on back navigation icon

Expected Result: Next button shouldn’t be disabled as long as there is a selected user
Actual Result: Next button is disabled even thought there is a selected user
Issue 2
Alluded to by @bondydaa here
Action Performed:

Log in with account that has access to the policy-expense-chat beta
Go offline by navigating to [profile icon] > Preferences > and enable Force offline
Create a new workspace.
Invite some people to the workspace. Verify that workspace chats for each member appear optimistically.
Go online by navigating to [profile icon] > Preferences > disable Force online.

Expected Result:
Workspace chats created for each members are updated accordingly
Actual Result:
??? Something else ???

@roryabraham
For issue 1, I am working on it and the fix is known https://github.com/Expensify/App/pull/15672#issuecomment-1528755205
For issue 2,

For issue 2,
From code walkthrough, The intersection i see between both PR which could have caused regression might be
https://github.com/Expensify/App/pull/16764/files#diff-7040f9d045bb4fd912718e140643e494268c688306bec7a1928376d9003566daR274-R276
            Policy.addMembersToWorkspace(filteredLogins, this.state.welcomeNote, this.props.route.params.policyID, this.props.betas);

We are not passing this.props.beta in the flow. Let me make the change and test it.

Created a draft PR with the fixes for issues - https://github.com/Expensify/App/pull/18395
Working on author checklist

@bondydaa Lets make sure to add the DeployBlockerCash to issues and not PRs to make the flow easier, it will autoassign hourly to an engineer and we can summarize more easily what the reproduction steps are, thanks! 🙇

@abdulrahuman5196 does your PR address both issues with this PR?

@luacmartins  Yes. I am addressing both the issues.
But I would require help testing if my fix is working for the issue 2 since it seems to be working before my change as per here - https://github.com/Expensify/App/pull/16764#issuecomment-1533977121

Hey team! not sure if this is on your radar but could you check to see if https://github.com/Expensify/App/issues/18390 is a regression of this?

Raised a Followup PR for the Fixes

🚀 Deployed to production by https://github.com/roryabraham in version: 1.3.12-0 🚀

platform
result

🤖 android 🤖
success ✅

🖥 desktop 🖥
success ✅

🍎 iOS 🍎
success ✅

🕸 web 🕸
success ✅

authors: Christinadobrzyn MelvinBot MonilBhavsar OSBotify Santhosh-Sellavel abdulrahuman5196 bondydaa luacmartins marcaaron mollfpr mountiny roryabraham thienlnam