Closed lanitochka17 closed 2 months ago
Triggered auto assignment to @mallenexpensify (Bug
), see https://stackoverflow.com/c/expensify/questions/14418 for more details.
Job added to Upwork: https://www.upwork.com/jobs/~01a86156cf627cbc84
Platforms
in OP are β
)Triggered auto assignment to Contributor-plus team member for initial proposal review - @eVoloshchak (External
)
from: @cooldev900
GooglePlacesAutocomplete shows old selected results in offline mode.
The route cause is that if the app turns to offline state, requestUrl.url becomes null and it couldn't send location filter request to google API.
const [url, setUrl] = useState(getRequestUrl(props.requestUrl));
const _request = (text) => {
_abortRequests();
if (!url) {
return;
const [tempSearchedValue, setTempsearchedValue] = useState([])
const buildRowsFromResults = useCallback(
(results, text) => {
let res = [];
const shouldDisplayPredefinedPlaces = text
? results.length === 0 && text.length === 0
: results.length === 0;
if (
shouldDisplayPredefinedPlaces ||
props.predefinedPlacesAlwaysVisible === true
) {
res = [
...props.predefinedPlaces.filter(
(place) => place?.description.length,
),
];
if (props.currentLocation === true && hasNavigator()) {
res.unshift({
description: props.currentLocationLabel,
isCurrentLocation: true,
});
}
}
res = res.map((place) => ({
...place,
isPredefinedPlace: true,
}));
return [...res, ...results];
},
[
props.currentLocation,
props.currentLocationLabel,
props.predefinedPlaces,
props.predefinedPlacesAlwaysVisible,
],
);
And then even if we change the input value, the request won't will be run in request function so dataSource state value doesn't change. As a result, GooglePlaceAutocomplete compoment shows old search result.
And if we don't search anything and it turns to offline, the predefined places shows in GooglePlaceAutocomplete.
Regarding discussion, we can filter recent destination by user's input in offline and send it as a props to GooglePlacesAutocomplete.
const filteredPredefinedPlaces = useMemo(() => {
console.log({predefinedPlaces: props.predefinedPlaces})
if (!props.network.isOffline || !searchValue) return props.predefinedPlaces.length < 5 ? props.predefinedPlaces : props.predefinedPlaces.slice(0, 5);
const filterRecentPlaces = props.predefinedPlaces.filter(value => !value?.description ? false :searchValue.split(" ").some((searchTerm) => value?.description.toLocaleLowerCase.includes(searchTerm.toLocaleLowerCase())))
return filterRecentPlaces.length < 5 ? filterRecentPlaces : filterRecentPlaces.slice(0, 5);
}, [
props.network.isOffline, props.predefinedPlaces, searchValue
])
return (
/*
* The GooglePlacesAutocomplete component uses a VirtualizedList internally,
* and VirtualizedLists cannot be directly nested within other VirtualizedLists of the same orientation.
* To work around this, we wrap the GooglePlacesAutocomplete component with a horizontal ScrollView
* that has scrolling disabled and would otherwise not be needed
*/
<>
<ScrollView
horizontal
contentContainerStyle={styles.flex1}
scrollEnabled={false}
// keyboardShouldPersistTaps="always" is required for Android native,
// otherwise tapping on a result doesn't do anything. More information
// here: https://github.com/FaridSafi/react-native-google-places-autocomplete#use-inside-a-scrollview-or-flatlist
keyboardShouldPersistTaps="always"
>
<View
style={styles.w100}
ref={containerRef}
>
<GooglePlacesAutocomplete
disableScroll
fetchDetails
suppressDefaultStyles
enablePoweredByContainer={false}
predefinedPlaces={filteredPredefinedPlaces}
_.map(waypoints ? waypoints.slice(0, 20) : [], (waypoint) => ({
Onyx.merge(ONYXKEYS.NVP_RECENT_WAYPOINTS, clonedWaypoints.slice(0, 20));
N/A
@cooldev900 This has not been merged yet https://github.com/Expensify/App/pull/29045
Can you send the video with your solution?
If this is an upstream bug, @FaridSafi might help you with this
I will upload the video.
ice_video_20231022-084232.webm
const filteredPredefinedPlaces = useMemo(() => {
if (!props.network.isOffline || !searchValue) return props.predefinedPlaces
return props.predefinedPlaces.filter(value => value?.description.toLocaleLowerCase().includes(searchValue.toLocaleLowerCase()))
}, [
props.network.isOffline, props.predefinedPlaces, searchValue
])
return (
/*
* The GooglePlacesAutocomplete component uses a VirtualizedList internally,
* and VirtualizedLists cannot be directly nested within other VirtualizedLists of the same orientation.
* To work around this, we wrap the GooglePlacesAutocomplete component with a horizontal ScrollView
* that has scrolling disabled and would otherwise not be needed
*/
<>
<ScrollView
horizontal
contentContainerStyle={styles.flex1}
scrollEnabled={false}
// keyboardShouldPersistTaps="always" is required for Android native,
// otherwise tapping on a result doesn't do anything. More information
// here: https://github.com/FaridSafi/react-native-google-places-autocomplete#use-inside-a-scrollview-or-flatlist
keyboardShouldPersistTaps="always"
>
<View
style={styles.w100}
ref={containerRef}
>
<GooglePlacesAutocomplete
disableScroll
fetchDetails
suppressDefaultStyles
enablePoweredByContainer={false}
predefinedPlaces={filteredPredefinedPlaces}
@shubham1206agra @FaridSafi As a alternative, I showed only how to search predefined places in SearchAddress.js component in offline mode. We can fix it successfully with @FaridSafi by modifying react-native-google-places-autocomplete.
@eVoloshchak, @mallenexpensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!
@eVoloshchak , can you please review the above proposal?
π£ It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? πΈ
@cooldev900, thank you for the proposal!
There are two ways we could go with this:
The problem with the first approach is that we'll be saving only a fraction of places (only those that the user has searched for earlier), so while offline this won't be very useful. Additionally, there will be no clear indication that the search is "restricted" while offline, it will essentially half-work, which is worse than not working at all imo
@mallenexpensify, what do you think?
I'm unsure, can you create a post in #expensify-open-source (since #expensify-bugs is being deprecated for External) and tag me in it? I'll cross-post in the internal distance room. Thx
@eVoloshchak, @mallenexpensify Whoops! This issue is 2 days overdue. Let's get this updated quick!
@eVoloshchak can you create the #expensify-open-source post then drop the link in here? Thx
@eVoloshchak @mallenexpensify this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!
π£ It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? πΈ
@eVoloshchak π above plz
Started a thread on Slack
@eVoloshchak, @mallenexpensify Whoops! This issue is 2 days overdue. Let's get this updated quick!
@eVoloshchak, @mallenexpensify Whoops! This issue is 2 days overdue. Let's get this updated quick!
Thanks for posting @eVoloshchak , if anyone's interested in working on this issue, please review the chat in #expensify-open-source
Looking back at the OP
Expected Result:
Search result should update according to entered letter
Actual Result:
Search list displays old searched results in offline mode
I think we've kinda derailed this convo into discussing what the best flow is. (which kinda makes sense cuz we want to have an understanding of how everything works).
So... do we want to show results that include the first letter that's typed or just stick with the list of most recent. I'm unsure how much it matters if a user is only able to view the 20 most-recent locations.
@eVoloshchak @mallenexpensify this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!
π£ It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? πΈ
do we want to show results that include the first letter that's typed
I think that would be better. It is indeed very minor, but won't be hard to implement, while being useful in some cases
@eVoloshchak @mallenexpensify I update my proposal following slack discussion.
One problem here is how to handle certain characters like 'Γ lesund' when filtering places while offline. While offline, we can handle certain amout of special characters in frontend but there is no way to convert it through an online service. Is it possible to add an additional field in the backend to place data for easy filtering? Or is it ok to ignore special characters in filtering places?
@cooldev900 good π and consideration. I think non-standard letters will be rare and, if there's only 20 locations to choose from, we don't need to get to particular about the search working perfectly because someone can find one of the twenty quickly.
π£ It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? πΈ
@eVoloshchak, @mallenexpensify Still overdue 6 days?! Let's take care of this!
@eVoloshchak can we move forward with @cooldev900 's proposal above? If not, please provide feedback, thx
Yes, let's proceed with @cooldev900's proposal
Or is it ok to ignore special characters in filtering places?
It is ok and I think we already do that for a search in a country picker, I'd be good to re-use this logic (we can work this out on the PR stage)
πππ C+ reviewed!
Triggered auto assignment to @chiragsalian, see https://stackoverflow.com/c/expensify/questions/7972 for more details.
proposal LGTM, feel free to create the PR @cooldev900
π£ @eVoloshchak Please request via NewDot manual requests for the Reviewer role ($500)
π£ @cooldev900 π 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 π
@lakchote Here is the PR.
Taking over the issue from Chirag as he authorized here.
Current assignee @mallenexpensify is eligible for the NewFeature assigner, not assigning anyone new.
Triggered auto assignment to @dubielzyk-expensify (Design
), see these Stack Overflow questions for more details.
The latest C+ comment on the PR requested some design changes, and this is a new feature, so would you please review the PR from a design perspective?
β π₯ Heads up, I'm going to be OOO working 0% from 1/1-1/12/24. I'll be back on 1/15/24. I'm planning to work 50% tomorrow 12/29. Please feel free to un-assign, re-assign, whatever. When I return I'll pick back up whatever I can and do my best to GSD in order of priority. I will also post all of my assigned issues in #engineering-chat in Slack to try to recruit volunteers.
The PR has kind of stalled out waiting for one last bug fix, so as I said in a comment there, I'm going to ask if someone from Callstack can pick it up and bring it over the finish line.
Posted on Slack here asking for a Callstack person to take it over.
I am Pedro Guerreiro from Callstack - expert contributor group. Iβd like to work on this job!
This issue has not been updated in over 15 days. @eVoloshchak, @cooldev900, @mallenexpensify, @neil-marcellini, @pac-guerreiro, @dubielzyk-expensify eroding to Monthly issue.
P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!
How's it going @pac-guerreiro? What can we do to speed this up?
@eVoloshchak Aaaand it should be working now on all platforms! π π
From yesterday, can you review again @eVoloshchak ? Thx
@neil-marcellini Sorry I missed your comment π It should be done by now, just need another review π
This issue has not been updated in over 15 days. @eVoloshchak, @cooldev900, @mallenexpensify, @neil-marcellini, @pac-guerreiro, @dubielzyk-expensify eroding to Monthly issue.
P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!
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.3.88-3 Reproducible in staging?: Yes Reproducible in production?: Yes 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: Applause - Internal Team Slack conversation:
Action Performed:
Expected Result:
Search result should update according to entered letter
Actual Result:
Search list displays old searched results in offline mode
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
Windows: Chrome
https://github.com/Expensify/App/assets/78819774/7a45a7ba-2f20-41fc-a547-20c8ad1ed097MacOS: Desktop
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @eVoloshchak