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

PCS front+back #487

Open
wants to merge 62 commits into
base: master
Choose a base branch
from
Open

PCS front+back #487

wants to merge 62 commits into from

Conversation

AaDalal
Copy link
Contributor

@AaDalal AaDalal commented Apr 15, 2023

  • fix accordion arrow not showing up
  • mobile view

Copy link
Contributor Author

@AaDalal AaDalal left a comment

Choose a reason for hiding this comment

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

  1. Can we call dump_redis_data at regular intervals?
  2. We should include @joyliu-q and @alxkp on:
  1. the changes to k8s/*
  2. the thing I mentioned above about calling dump_redis_data regularly
  3. making sure that our redis cluster will stay up and preserve state between when we call dump_redis_data (platform needs to guarantee this)
  1. Frontend is still a mystery 😨
  2. We need to lint :)
  3. Happy to call to go over this stuff (I can also help fix anything if needed)

backend/PennCourses/settings/base.py Show resolved Hide resolved
backend/courses/management/commands/dump_redis_data.py Outdated Show resolved Hide resolved
)
avg_reviews = reviewed_course["average_reviews"]
yield {
"code": course.full_code.replace("-", " "),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

is there a reason that we replace the dashes? Does redisearch not do stemming unless we do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rohangpta or @rm03 do you know?

Copy link
Member

Choose a reason for hiding this comment

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

Not sure, my guess is as good as yours. @rm03?

Copy link
Member

Choose a reason for hiding this comment

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

Also unsure - RediSearch won't stem that field as specified in the schema

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we should stem that field. It's pretty bad at matching course codes rn :/

backend/courses/management/commands/dump_redis_data.py Outdated Show resolved Hide resolved
backend/courses/management/commands/dump_redis_data.py Outdated Show resolved Hide resolved
frontend/review/src/components/DeepSearch/CoursePreview.js Outdated Show resolved Hide resolved
frontend/review/src/components/DeepSearch/DeepSearchBar.js Outdated Show resolved Hide resolved
frontend/review/src/components/SearchBar.js Outdated Show resolved Hide resolved
k8s/main.ts Outdated Show resolved Hide resolved
k8s/main.ts Outdated Show resolved Hide resolved
Copy link
Contributor Author

@AaDalal AaDalal left a comment

Choose a reason for hiding this comment

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

Just some more clarifications

id__in=Subquery(Section.objects.filter(section_filters_pcr).values("instructors__id"))
)
.distinct()[:100]
.values("name", "id", "section__course__department__code")
Copy link
Contributor Author

@AaDalal AaDalal Aug 11, 2023

Choose a reason for hiding this comment

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

^^ There can be more than 1 associated section, but you can use section to get all of them (like section_set). For some reason we named it section not section_set

backend/courses/management/commands/dump_redis_data.py Outdated Show resolved Hide resolved
backend/courses/management/commands/dump_redis_data.py Outdated Show resolved Hide resolved
@AaDalal AaDalal marked this pull request as ready for review August 15, 2023 02:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

6 participants