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

feat: create API for questions search #3996

Merged
merged 6 commits into from
Dec 11, 2024

Conversation

onim-at
Copy link
Contributor

@onim-at onim-at commented Nov 9, 2024

PR Checklist

PR Type

What kind of change does this PR introduce?

  • Bugfix (fixes an issue)
  • Feature (adds functionality)
  • Code style update
  • Refactoring (no functional changes)
  • Build related changes
  • CI related changes
  • Documentation changes
  • Other... Please describe:

What is the current behavior?

Fetch questions via firestore

What is the new behavior?

Fetch questions via remix backend API

Does this PR introduce a breaking change?

  • Yes
  • No

Git Issues

Closes #3917

What happens next?

Thanks for the contribution! We try to make sure all PRs are reviewed ahead of our monthly maintainers call (first Monday of the month)

If the PR is working as intended it'll be merged and included in the next platform release, if not changes will be requested and re-reviewed once updated.

If you need more immediate feedback you can try reaching out on Discord in the Community Platform development channel.

@onim-at onim-at force-pushed the 3917-question-search-remix branch from 030f316 to 1a01427 Compare November 11, 2024 20:05
@onim-at onim-at marked this pull request as ready for review November 11, 2024 20:06
@onim-at onim-at requested a review from a team as a code owner November 11, 2024 20:06
@onim-at onim-at force-pushed the 3917-question-search-remix branch from 1a01427 to 10f4d08 Compare November 11, 2024 20:07
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this file is under a test folder because otherwise it would raise the following error:

Error: Vitest failed to access its internal state.

One of the following is possible:
- "vitest" is imported directly without running "vitest" command
- "vitest" is imported inside "globalSetup" (to fix this, use "setupFiles" instead, because "globalSetup" runs in a different context)
- Otherwise, it might be a Vitest bug. Please report it to https://github.com/vitest-dev/vitest/issues

Copy link
Contributor

@mariojsnunes mariojsnunes Nov 11, 2024

Choose a reason for hiding this comment

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

same as for research, here:
#3987 (comment)

these tests are no longer useful. can remove the file, but add some cypress tests.

@onim-at onim-at force-pushed the 3917-question-search-remix branch from 10f4d08 to c1f0579 Compare November 24, 2024 19:16
Copy link

cypress bot commented Nov 24, 2024

onearmy-community-platform    Run #6671

Run Properties:  status check passed Passed #6671  •  git commit 2b7b2aa149: Merge branch 'master' into 3917-question-search-remix
Project onearmy-community-platform
Branch Review pull/3996
Run status status check passed Passed #6671
Run duration 05m 04s
Commit git commit 2b7b2aa149: Merge branch 'master' into 3917-question-search-remix
Committer benfurber
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 1
Tests that did not run due to a developer annotating a test with .skip  Pending 0
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 74
View all changes introduced in this branch ↗︎

@benfurber benfurber added the Review allow-preview ✅ Has received manual check for malicious code and can be safely built for preview label Nov 27, 2024
@benfurber
Copy link
Member

benfurber commented Nov 28, 2024

Thanks for doing this @onim-at and especially for the cypress tests! I'll comment on them seperately, first the QA-ing though. I'm getting weird behaviour.

  1. The search seems to work for flash and then clears?
    https://github.com/user-attachments/assets/9df9b446-c1ec-44e7-8532-6acc1ebd7efd

  2. I'm not even sure what's going on with the orders...
    https://github.com/user-attachments/assets/3e2848fe-30f0-41d8-802f-0719be5ec01b

I'll add some categories to the preview DB too so I can check the categories.

@benfurber
Copy link
Member

Added a category. Clicking on one doesn't seem to do anything. i.e. it doesn't select the option, add to the path or filter the list.

@onim-at
Copy link
Contributor Author

onim-at commented Dec 7, 2024

Hello @benfurber! Thanks for the review and QA, I tried to reproduce the issues locally

  1. The search seems to work for flash and then clears?

That was a tricky bug! The const params = new URLSearchParams(request.url) inside api.questions.ts was creating the following object, mapping incorrectly the first search parameters

URLSearchParams {
  'http://localhost:3000/api/questions?words' => 'test',
  'category' => '',
  'sort' => 'MostRelevant',
  'lastDocId' => '' 
}

I fixed it by extracting first the URL, then the search params

const url = new URL(request.url)
const params = new URLSearchParams(url.search)
  1. I'm not even sure what's going on with the orders...

I couldn't reproduce it locally, it looks ok to me😞
https://github.com/user-attachments/assets/72d3383c-a6da-421a-a0cd-122c46ef10a5

  1. filtering categories

Same thing for the categories, it is working properly on my local
https://github.com/user-attachments/assets/9e9965cb-53b5-4aa0-962c-e703b1542637

@benfurber
Copy link
Member

Nice investigation on the search bug!

I had a play with this locally using PP prod data (minus the new commenting stuff as I don't have all the prod creds I'd need for the env easily to hand).

We've removed the comment ordering anyway for the moment so I think this can go through.

Thanks @onim-at!

@benfurber benfurber merged commit dec32a1 into ONEARMY:master Dec 11, 2024
18 checks passed
@onim-at onim-at deleted the 3917-question-search-remix branch December 11, 2024 15:23
@onearmy-bot
Copy link
Collaborator

🎉 This PR is included in version 2.17.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released Review allow-preview ✅ Has received manual check for malicious code and can be safely built for preview
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[Tech] Create API for questions search
4 participants