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.

Sign up for GitHub

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

Jump to bottom

Include long links into search #998

Merged
merged 5 commits into from
Dec 8, 2020
Merged

Include long links into search #998

merged 5 commits into from
Dec 8, 2020

Conversation

Oxiang
Copy link
Contributor

@Oxiang Oxiang commented Dec 2, 2020

Problem

Currently, users are only able to search keywords based on short links and description. However, users are also searching long links as recorded by google analytics.

Partially address #977

Solution

Improvements:

  • Add longUrl as part of search vector
  • Script for db migration to include longUrl into the search index
  • description ranking is dropped to the lowest as they are no longer being filled by users
  • shortUrl ranking still remains at the top as they are the most frequently used keywords

@Oxiang
perf: include longurl to search
6b5e5a6 
- add longurl into search vector with B ranking
- lower description ranking to C
- include db migration script
liangyuanruo
liangyuanruo previously requested changes Dec 2, 2020
View reviewed changes
Copy link
Contributor

@liangyuanruo liangyuanruo left a comment

Choose a reason for hiding this comment

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

actually, instead of dropping the existing index, could we have both the old and new indexes co-exist? this will allow the database to support both old and new application versions, which will be necessary if there is a rollback.

@Oxiang
perf: keep old index
bb9b2d8 
- remove dropping of old search index incase of rollback
- new db migration script to create new search index
- change sequelize model to create new search index in development
LoneRifle
LoneRifle requested changes Dec 2, 2020
View reviewed changes
src/server/models/url.ts Outdated Show resolved Hide resolved
Oxiang added 3 commits December 2, 2020 17:23
@Oxiang
fix: rename index
c3431be 
- change new_urls_weighted_search_idx to urls_combined_weighted_search_idx
@Oxiang
fix: rename db migration file
7261a2b
@Oxiang
fix: add drop if exists statement
f0d81b9
LoneRifle
LoneRifle approved these changes Dec 8, 2020
View reviewed changes
Copy link
Contributor

@LoneRifle LoneRifle left a comment

Choose a reason for hiding this comment

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

lgtm

@yong-jie yong-jie dismissed liangyuanruo’s stale review December 8, 2020 05:48

requested changes have been addressed

@yong-jie yong-jie merged commit 90155f6 into develop Dec 8, 2020
@yong-jie yong-jie deleted the longurl-search branch December 8, 2020 05:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LoneRifle LoneRifle LoneRifle approved these changes

@liangyuanruo liangyuanruo liangyuanruo left review comments

@yong-jie yong-jie Awaiting requested review from yong-jie

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Oxiang @liangyuanruo @LoneRifle @yong-jie