Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CAS-541: Search Endpoint #574

Merged
merged 8 commits into from
Oct 7, 2022
Merged

CAS-541: Search Endpoint #574

merged 8 commits into from
Oct 7, 2022

Conversation

jbluks
Copy link
Collaborator

@jbluks jbluks commented Oct 5, 2022

  • Fix the endpoint to be /search?text=search+text to allow for spaces. If text is blank, the default search for featured communities will be performed
  • Bugs in the filter counts also fixed.

@jbluks jbluks requested a review from germanurrus October 5, 2022 17:11
@linear
Copy link

linear bot commented Oct 5, 2022

CAS-541

@jbluks jbluks requested a review from 0xmovses October 5, 2022 17:11
Copy link
Contributor

@germanurrus germanurrus left a comment

Choose a reason for hiding this comment

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

Looking good! Thanks for the tweaks in the frontend! Made some comments

frontend/packages/client/src/pages/BrowseCommunities.js Outdated Show resolved Hide resolved
resultsWithCount = append(resultsWithCount, community)
}

return resultsWithCount
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like the count url param is not taken into account on the search result, I've created a set for testing and results array has more elements than the count number on the first pull from the backend (FE sends 10 and backend returns all results):

Screen Shot 2022-10-06 at 11 21 34

The other thing I noticed is that results are not filtered with the text search after the space, in this case "before 2" should return only the community that has "before 2" but it returns the same as if I would enter "before" (The name of the community makes no sense :( ). May it has to do with the configuration of the search plug-in?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed the search results count/start. Was missing from the search text query. The results are inclusive of any of the words, so because the search contains "before" it will return any community with that word, even though the search contains another search string. The search does not "narrow" with new terms. I think this makes sense for a browser page.

@jbluks jbluks force-pushed the jonathan+rich/cas-541 branch from df26259 to de2a1d0 Compare October 6, 2022 19:16
@germanurrus
Copy link
Contributor

there's one last glitch while filtering, if the frontend sends the filter results should be narrowed but the filter should return the same amount originally created for the text search:

Screen.Recording.2022-10-06.at.16.33.26.mov

...(filters ? { filters: filters } : undefined),
start,
count,
}).toString();

const url = `${process.env.REACT_APP_BACK_END_SERVER_API}/communities/search/${searchText}?${queryParams}`;
const url = `${process.env.REACT_APP_BACK_END_SERVER_API}/communities/search?${queryParams}`;

const response = await fetch(url);
return checkResponse(response);
Copy link
Contributor

Choose a reason for hiding this comment

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

on line 70:

 getNextPageParam: (lastPage) => {
          // custom adjustment
          const { next, start, count, totalRecords } = lastPage?.results ?? {};
          return [start + count, count, totalRecords, next];
        },

@jbluks jbluks force-pushed the jonathan+rich/cas-541 branch from 1e5fe46 to 7574446 Compare October 6, 2022 23:18
@jbluks jbluks force-pushed the jonathan+rich/cas-541 branch from 7574446 to 61e1694 Compare October 6, 2022 23:18
Copy link
Contributor

@germanurrus germanurrus left a comment

Choose a reason for hiding this comment

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

Tested locally and works as expected!

@jbluks jbluks merged commit 5d9136b into release-v1.5.0 Oct 7, 2022
@jbluks jbluks deleted the jonathan+rich/cas-541 branch October 7, 2022 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants