-
-
Notifications
You must be signed in to change notification settings - Fork 399
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
Create API for research search #3987
base: master
Are you sure you want to change the base?
Conversation
…tt12/community-platform into 3924-research-remix-backend
onearmy-community-platform Run #6538
Run Properties:
|
Project |
onearmy-community-platform
|
Branch Review |
pull/3987
|
Run status |
Failed #6538
|
Run duration | 05m 37s |
Commit |
8760cc630c: fix: fix PR reviews
|
Committer | unknown |
View all properties for this run ↗︎ |
Test results | |
---|---|
Failures |
2
|
Flaky |
1
|
Pending |
0
|
Skipped |
0
|
Passing |
70
|
View all changes introduced in this branch ↗︎ |
Tests for review
settings.spec.ts • 1 failed test • ci-chrome
Test | Artifacts | |
---|---|---|
[Settings] > [Fixing Fashion] > Can create member |
Test Replay
Screenshots
Video
|
map.spec.ts • 1 failed test • ci-chrome
Test | Artifacts | |
---|---|---|
[Map] > [Shows expected pins] |
Test Replay
Screenshots
Video
|
src/integration/research/follow.spec.ts • 1 flaky test • ci-chrome
Test | Artifacts | |
---|---|---|
[Research] > [By Authenticated] > [Follow button] |
Test Replay
Screenshots
Video
|
|
||
describe('research.service', () => { | ||
describe('search', () => { | ||
it('fetches research articles based on search criteria', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these tests aren't very useful now, because it's just mocking and checking the mocked value, so not testing anything real.
we should instead add cypress and/or UI tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Im on it :)
constraints = [...constraints, sortConstraint] | ||
try { | ||
const response = await fetch( | ||
`/api/research?words=${words.join(',')}&category=${category}&sort=${sort}&status=${status ?? ''}&last_doc_id=${lastDocId ?? ''}`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would be cleaner to use URLSearchParams
https://developer.mozilla.org/en-US/docs/Web/API/URLSearchParams/append
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, thank you for the idea
|
||
import type { ICategory } from 'oa-shared' | ||
|
||
const cache = new Keyv<ICategory[]>({ ttl: 600000 }) // ttl: 10 minutes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this rarely changes, should increase for at least 1h
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
export const loader = async ({ request }) => { | ||
const url = new URL(request.url) | ||
const searchParams = url.searchParams | ||
const userId: string | null = searchParams.get('user_id') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these params are usualy camelCase, so userId
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, on the API side as well
export const loader = async () => { | ||
const cachedCategories = await cache.get('researchCategories') | ||
|
||
// check if cached map pins are availbe, if not - load from db and cache them |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
map pins
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops :)
const cachedCategories = await cache.get('researchCategories') | ||
|
||
// check if cached map pins are availbe, if not - load from db and cache them | ||
if (cachedCategories) return json({ categories: cachedCategories }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the brackets
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What brackets do you mean?
PR Checklist
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Fetching of research articles is being done directly with Firestore
What is the new behavior?
Fetching of research articles have moved to a Remix backend API
If useful, provide screenshot or capture to highlight main changes
Does this PR introduce a breaking change?
Git Issues
Closes #3924
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.