Improve Search Functionality #172

TiagooGomess · 2022-02-11T22:51:19Z

2022-02-14.19-15-39.mp4

Closes #79

TiagooGomess · 2022-03-24T20:50:24Z

The only thing missing is the implementation of new tests.

imnotteixeira

I still have to review the whole system (the refetching and all) just checked some high level stuff

src/components/HomePage/SearchArea/SearchArea.js

src/components/HomePage/SearchResultsArea/Offer/OfferItem.js

src/components/HomePage/SearchResultsArea/SearchResultsWidget/SearchResultsUtils.js

TiagooGomess · 2022-03-28T17:50:24Z

src/components/HomePage/SearchResultsArea/SearchResultsWidget/useOffersSearcher.js

+    // The "search" and "loadMoreOffers" functions do not share the same state;
+    // When we run "setHasMoreOffers(false)" on the "loadMoreOffers" function,
+    // the "search" function does not know that the "hasMoreOffers" variable has changed;
+    // In the same way, when we run "setHasMoreOffers(true) on the "search" function,
+    // the "loadMoreOffers" function does not know that the "hasMoreOffers" variable has changed;
+    // Then, when the "loadMoreOffers" function is executed after a previous execution where the
+    // "hasMoreOffers" variable became "false", the state of this variable is still false, which
+    // prevents the fetching of new offers.
+    // Knowing that, I needed a way to set the "hasMoreOffers" variable to "true" when the previous fact
+    // happens: setting the "hasMoreOffers" to true when the "queryToken" (which is stored in redux) changes
+    useEffect(() => {
+        setHasMoreOffers(true);
+    }, [searchQueryToken]);


@imnotteixeira I found out that we have a bug when we fetch all the offers of a search and then do another search. What do you think of this way to solve that?

I have a solution, but I'm not sure you're going to like it...

Maybe it's time to go full-hook and ditch redux for search functionality.

The main idea is: the HomePage Controller (might need to create it) will start storing the search state by calling the search hook. The search hook will be modified to hold the state and export the setters of each filter and the search value etc. Then it will expose the search and loadMore functions, as it does currently. It will additionally hold the results array, and errors and loading state, etc... like the redux state currently has.

Then SearchArea and SearchResultsWidget components can both access the HomePage context which will have these methods/data accessible, and most importantly common to both. There can only be 1 search at any given time, which is what we want. Components that currently use the redux state for search can easily call useContext(HomePageContext) and retrieve whatever they need, no need to pass props around, no connect components to redux.

I'm not sure if we will only ever have the search on the homepage. Maybe some day we will want to keep the search state while the student goes through other pages (viewing offers, adding them to his personal track-list, dunno) and keep the results in a sort-of cart that they can quickly get back to. But for now this is good enough, no need to handle the context above for it to be accessed in other pages.

I think it's a good solution

@imnotteixeira That refactoring will take some time, mainly due to all the changes in the tests. What do you think of leaving this as it is now (because it works) and opening an issue for that?

@imnotteixeira I found out that we have a bug when we fetch all the offers of a search and then do another search. What do you think of this way to solve that?

Then is this a bug or not?

In any case, we should migrate in the future. Search is the next big system still not using hooks/context. Migrating will probably make it simpler. Create the issue and link it here please

It was a bug, but I solved it using that useEffect, which uses the searchQueryToken from redux.

imnotteixeira · 2022-04-03T18:06:59Z

Beware that search service is being changed here #217. Make sure the changes are migrated to this PR as well. Rebases can sometimes hide these changes and the merging goes wrong.

…ll variables

…on do not share state

monkin77

Apart from the problem that was discussed and will be solved in another issue, this looks great! I don't have my PC to run this locally but from the video and code review it's 💯.

src/components/HomePage/SearchResultsArea/SearchResultsWidget/useOffersSearcher.spec.js

TiagooGomess · 2022-05-24T17:52:51Z

@imnotteixeira Sorry to bother you, but since you requested changes in this PR, we need your review so that this PR is ready for merging.

imnotteixeira

I can't really review currently, so I'll approve since there is already another approval to avoid blocking

TiagooGomess added this to the MVP milestone Feb 11, 2022

TiagooGomess self-assigned this Feb 11, 2022

TiagooGomess force-pushed the feature/improve-search-functionality branch 3 times, most recently from 6127cb8 to 7c6dd69 Compare February 14, 2022 18:36

TiagooGomess force-pushed the feature/improve-search-functionality branch from 5428133 to 5ba7b6c Compare February 22, 2022 19:38

BrunoRosendo mentioned this pull request Mar 20, 2022

Changed search system to keyset-based NIAEFEUP/nijobs-be#212

Merged

TiagooGomess force-pushed the feature/improve-search-functionality branch from 6cc22a6 to 879a332 Compare March 24, 2022 19:30

imnotteixeira requested changes Mar 25, 2022

View reviewed changes

src/components/HomePage/SearchArea/SearchArea.js Outdated Show resolved Hide resolved

src/components/HomePage/SearchResultsArea/Offer/OfferItem.js Show resolved Hide resolved

src/components/HomePage/SearchResultsArea/SearchResultsWidget/SearchResultsUtils.js Outdated Show resolved Hide resolved

TiagooGomess force-pushed the feature/improve-search-functionality branch 3 times, most recently from ed7664d to c12ecd1 Compare March 26, 2022 19:39

TiagooGomess commented Mar 28, 2022

View reviewed changes

TiagooGomess force-pushed the feature/improve-search-functionality branch 2 times, most recently from 4dcd572 to cbf11ee Compare March 28, 2022 18:33

TiagooGomess mentioned this pull request Apr 1, 2022

Migrate HomePage state from redux to hooks/context #215

Open

TiagooGomess linked an issue Apr 1, 2022 that may be closed by this pull request

Migrate HomePage state from redux to hooks/context #215

Open

TiagooGomess force-pushed the feature/improve-search-functionality branch from cbf11ee to 1255f0f Compare April 6, 2022 18:01

This was referenced Apr 6, 2022

feat: add event for impression metrics #223

Merged

Metrics - Register offers in search results #181

Closed

TiagooGomess force-pushed the feature/improve-search-functionality branch 5 times, most recently from 892e91e to dc7e8ce Compare April 13, 2022 14:11

TiagooGomess added 18 commits April 20, 2022 21:15

Add: submitNumber property to the redux state, to reset infinite scro…

c67e63d

…ll variables

Add: search offers infinite scrolling working correctly

6f7f93e

Remove: unnecessary variables

f4ffa45

Fix: warning in mobile view regarding PropTypes

9895fbd

Fix: infinite scroll loading display

217fa40

Refactor: renamed variables

91fbe5e

Refactor: moved infinite scroll to SearchResultsController

46bf5ef

Update: offer search offset logic to queryToken

d20f73d

Fix: warning in OfferItem Skeleton circle attribute

8b16734

Fix: offer search tests

5b42124

Add: requested changes

9c2282f

Refactor: offer search

ac53e0f

Fix: load more offers in search bug

61624ab

Refactor: infinite scrolling is now using scroll percentage as a trigger

9df8c3f

Fix: load more offers when there is no scroll

4327588

Fix: bug regarding the fact that the search and loadMoreOffers functi…

3d62263

…on do not share state

Fix: search offers tests

127781d

Add: test for load more offers feature

ecb6b42

TiagooGomess force-pushed the feature/improve-search-functionality branch from 98405dd to ecb6b42 Compare April 20, 2022 20:15

imnotteixeira removed their request for review May 3, 2022 21:15

DoStini requested a review from monkin77 May 11, 2022 16:09

monkin77 approved these changes May 18, 2022

View reviewed changes

TiagooGomess commented May 19, 2022

View reviewed changes

src/components/HomePage/SearchResultsArea/SearchResultsWidget/useOffersSearcher.spec.js Outdated Show resolved Hide resolved

Remove: unused file

b004555

imnotteixeira approved these changes May 24, 2022

View reviewed changes

TiagooGomess merged commit 0458ebc into develop May 24, 2022

TiagooGomess deleted the feature/improve-search-functionality branch May 24, 2022 18:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve Search Functionality #172

Improve Search Functionality #172

TiagooGomess commented Feb 11, 2022 •

edited

Loading

TiagooGomess commented Mar 24, 2022

imnotteixeira left a comment

TiagooGomess Mar 28, 2022

imnotteixeira Mar 28, 2022

TiagooGomess Mar 28, 2022 •

edited

Loading

TiagooGomess Apr 1, 2022

imnotteixeira Apr 1, 2022

TiagooGomess Apr 1, 2022

imnotteixeira commented Apr 3, 2022

monkin77 left a comment

TiagooGomess commented May 24, 2022

imnotteixeira left a comment

Improve Search Functionality #172

Improve Search Functionality #172

Conversation

TiagooGomess commented Feb 11, 2022 • edited Loading

TiagooGomess commented Mar 24, 2022

imnotteixeira left a comment

Choose a reason for hiding this comment

TiagooGomess Mar 28, 2022

Choose a reason for hiding this comment

imnotteixeira Mar 28, 2022

Choose a reason for hiding this comment

TiagooGomess Mar 28, 2022 • edited Loading

Choose a reason for hiding this comment

TiagooGomess Apr 1, 2022

Choose a reason for hiding this comment

imnotteixeira Apr 1, 2022

Choose a reason for hiding this comment

TiagooGomess Apr 1, 2022

Choose a reason for hiding this comment

imnotteixeira commented Apr 3, 2022

monkin77 left a comment

Choose a reason for hiding this comment

TiagooGomess commented May 24, 2022

imnotteixeira left a comment

Choose a reason for hiding this comment

TiagooGomess commented Feb 11, 2022 •

edited

Loading

TiagooGomess Mar 28, 2022 •

edited

Loading