Closed m-natarajan closed 4 months ago
Triggered auto assignment to @stephanieelliott (Bug
), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.
Triggered auto assignment to @sakluger (Bug
), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.
Reapplying the Bug
label to get another BZ member on this while I am OOO til May 2. Thanks @sakluger, I'll grab this back from you when I return!
Extra bottom margin below green button in Personal info page
on ChatFinderPage
, what ever the search inside the input, we turn results only from existing reports.
We always turn Null
to userToInvite
https://github.com/Expensify/App/blob/944751fcec525ece0da16a6a4efd80ad8ab4ff8c/src/pages/ChatFinderPage/index.tsx#L109
as we do on NewChatPage, https://github.com/Expensify/App/blob/944751fcec525ece0da16a6a4efd80ad8ab4ff8c/src/pages/NewChatPage.tsx#L55
We need to use getFilteredOptions
with the debouncedSearchTerm
to get back the new userToInvite
update here to:
const filteredOptions = useMemo(() => {
if (debouncedSearchValue.trim() === '') {
return {
recentReports: [],
personalDetails: [],
userToInvite: null,
headerMessage: '',
};
}
const optionsWithSearch = OptionsListUtils.getSearchOptions(options, debouncedSearchValue, betas ?? []);
const newOptions = OptionsListUtils.filterOptions(optionsWithSearch, debouncedSearchValue);
const header = OptionsListUtils.getHeaderMessage(newOptions.recentReports.length > 0, false, debouncedSearchValue);
return {
recentReports: newOptions.recentReports,
personalDetails: newOptions.personalDetails,
userToInvite: optionsWithSearch.userToInvite,
headerMessage: header,
};
}, [betas, debouncedSearchValue, options]);
We can also combine optionsWithSearch
with newOptions
to use one function
POC:
https://github.com/Expensify/App/assets/12425932/b18c2514-8fb8-403a-8d94-16674cea8708
Job added to Upwork: https://www.upwork.com/jobs/~01c732e2d542ec4ad2
Triggered auto assignment to Contributor-plus team member for initial proposal review - @ntdiary (External
)
Upwork job price has been updated to $500
No results found error message appears
We always return userToInvite
option as null
in ChatFinderPage
here. So this page will never show the new user option when searching.
In filterOptions
function, if both recentReports
and personalDetails
are empty, we can create a userToInvite
option. We can do this as the same way we do in getOptions
function here and return this option as userToInvite
here.
After that on ChatFinderPage
, we will return userToInvite
as newOptions. userToInvite
instead of return null here
*Note: We should not calling getSearchOptions
again in this case because we only want to use searchOptions
which contain all options to filter the option via search input value.
NA
Unable to start chat with new user from new chat finder page.
The changes to implement filtering in search page https://github.com/Expensify/App/pull/37909, involved creation of a new function, libs/OptionsListUtils.filterOptions, replicating/replacing some logic previously performed in libs/OptionsListUtils.getOptions. It looks like the logic to create an option to invite a new user simply was not considered in the new function.
The data needed for the option to invite a new user is currently created in getOptions, and is named userToInvite. Pull the functionality for creating that data from getOptions into its own new function. Then have both getOptions and filterOptions call the new function. Then, filterOptions can return the data, and the chat finder page can use it.
The condition for creating that data is complicated (shown below). Make sure that each aspect of this condition is considered.
@DylanDylann will take over as c+. :)
I will review and give an update today
I don't think @dragnoir's proposal is good, we shouldn't call getSearchOptions
with the searchValue
because of performance (we only should call this function with searchValue
is empty).
@kmbcook's proposal and @nkdengineer's proposal have the same idea to create userToInvite
in
filterOptions
. Let's go with @nkdengineer's proposal because they are first
πππ C+ reviewed
Triggered auto assignment to @marcaaron, see https://stackoverflow.com/c/expensify/questions/7972 for more details.
@DylanDylann Please check again, I said:
We can also combine optionsWithSearch with newOptions to use one function
I didn't force using getSearchOptions. My RCA is correct and my brief description of the solution is correct.
As you mentioned:
idea to create userToInvite infilterOptions
This is exactly what I said in my proposal
I see that in your demo code
We can also combine optionsWithSearch with newOptions to use one function
Oh I see, in the next time, it will be clearer if you can split it to alternative solution and give more detail
@DylanDylann ALternative means another solution. I described my solutions in steps. And this is a proposal, not the final PR.
I mentioned a brief description about the way to solve the issue then I said, for the performance:
We can also combine optionsWithSearch with newOptions to use one function
I can't create the new function in the proposal, but I described it as you mentioned here https://github.com/Expensify/App/issues/40731#issuecomment-2075421143
Yeah, I see your idea. But for me, it does not make sense to accept a proposal with one line like this. It would be better if you could detail it more
We can also combine optionsWithSearch with newOptions to use one function
Anyway, It is only my decision. Everything is clear and let's wait for the thought from the internal engineer
Yeah, I see your idea. But for me, it does not make sense to accept a proposal with one line like this. It would be better if you could detail it more
How many lines do you see on the proposal you picked? It's also one line about the idea we are discussing here
@marcaaron, could you please review the proposals once more? I believe the decision made was not fair.
The C+ mentioned a preference for the 'idea to create userToInvite in filterOptions' to enhance performance, which is precisely what I proposed and detailed here.
Additionally, when I discussed this with the C+, he acknowledged that it was included in my proposal but suggested it should be in the alternative solutions section.
I explained that it was the same idea and that an alternative solution was unnecessary. However, he stated that he couldn't select a solution with one line. Please review my proposal, which includes multiple lines, detailed demo code, and a proof of concept video. In contrast, the selected proposal consists of just one line, with no code, demo, or video provided.
I urge you to thoroughly investigate this matter and ensure that decisions within the contributor program are made with fairness and integrity. I have spent countless hours searching for and crafting meaningful proposals, and I expect the decision-making process to reflect this commitment.
As said before
we shouldn't call getSearchOptions with the searchValue because of performance (we only should call this function with searchValue is empty).
Please review my proposal, which includes multiple lines, detailed demo code, and a proof of concept video. In contrast, the selected proposal consists of just one line, with no code, demo, or video provided.
It isn't correct. In your proposal, you gave the demo code and detailed implementation of the wrong direction and there is only one line is close to my direction
We can also combine optionsWithSearch with newOptions to use one function
It is not necessary to always need demo code and video provision. I expect your ideas are correct and clear enough
I have spent countless hours searching for and crafting meaningful proposals, and I expect the decision-making process to reflect this commitment.
Regarding this, everyone spends time preparing the proposal, not just you, and I try my best to be as fair as possible. @dragnoir we should minimize discussion here and wait for @marcaaron to give the final decision
@DylanDylann you repeat telling "one line", nkdengineer's idea is also just one line and it's the same as mine.
I expect your ideas are correct and clear enough
Yes it's correct, and clear. I created a quick demo and I mentioned that it will be enhanced. I don't have to create the final PR now.
This is what nkdengineer said:
We can do this as the same way we do in getOptions
I also mentioned this here:
I you did more, it's the same getOptions
Also when nkdengineer said:
return this option as userToInvite
I also mentioned:
to get back the new userToInvite
And about your idea, I said:
We can also combine optionsWithSearch with newOptions to use one function
I think this is clear now!!
Sorry, but this not fair and now I had a feeling that you just try to avoid the truth.
Sorry, but this not fair and now I had a feeling that you just try to avoid the truth.
The conversation goes far, let's stop here and wait for the final decision from the internal engineer
@DylanDylann I apologize if my previous message came across as impolite or upset you in any wayβthat was not my intention. I want to clarify that my concerns stem from the belief that the solution I proposed earlier was similar to the one selected, including the performance aspects I had also addressed. I hope we can resolve this amicably.
Here's a visual comparison to help you understand what I mentioned and you can see how the proposal picked is similar to mine and copy all my ideas.
You can see how similar colors are about similar notes.
Sorry again.
@nkdengineer is assigned to 53 issues at the moment. I am having trouble imagining how they could prioritize more work.
I'd rather work with @kmbcook on this one as they are assigned to 0.
@nkdengineer do you have any problem with that?
@dragnoir I have not read most of the comments here beyond this one. My feedback from the other issue might also be useful to you in this situation. If we screw up or get something wrong then please feel free to make a claim outside the issue. Someone will do their best to sort it out. Please keep the issue comments related to the problem we are trying to solve if possible π
@nkdengineer is assigned to 53 issues at the moment. I am having trouble imagining how they could prioritize more work.
@marcaaron Hi, most of them were actually merged/on HOLD. I'm not sure why after merged many issues were not updated with "Awaiting payment", maybe something wrong with the automation.
There's only around 16 open PRs from me and I had no trouble keeping them up to date every day if something is needed from me.
So I'm comfortable taking this one and can open the PR within today.
Thanks, see you in the reviews.
π£ @nkdengineer π An offer has been automatically sent to your Upwork account for the Contributor role π Thanks for contributing to the Expensify app!
Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review π§βπ» Keep in mind: Code of Conduct | Contributing π
@DylanDylann The PR is here.
Thanks for watching this while I was OOO, grabbing this back from you now @sakluger!
Looks like the PR is awaiting some changes requested of @nkdengineer
PR is merged, undergoing QA and should go to staging on next deploy
π£ @DylanDylann π An offer has been automatically sent to your Upwork account for the Contributor role π Thanks for contributing to the Expensify app!
Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review π§βπ» Keep in mind: Code of Conduct | Contributing π
This was deployed to prod on 5/8, for some reason the automation didn't work on this one. 7-day hold has already passed, so payment is due now:
Summarizing payment on this issue:
Contributor+: @DylanDylann $500 via Upwork - PAID
Upwork job is here: https://www.upwork.com/jobs/~01c732e2d542ec4ad2
@stephanieelliott I accepted the offer
All paid, thanks!
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.64-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: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Expensify/Expensify Issue URL: Issue reported by: @quinthar Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1713813518935569
Action Performed:
Expected Result:
Should be able to start chat with the new user
Actual Result:
No results found error message appears
Workaround:
unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Add any screenshot/video evidence
https://github.com/Expensify/App/assets/38435837/6ce69b09-baaa-44c0-a863-ba05783b115e
View all open jobs on GitHub
Upwork Automation - Do Not Edit