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: index tweaks and exclude blank description from search #277

Merged
merged 10 commits into from
Jul 20, 2020

Conversation

JasonChong96
Copy link
Contributor

@JasonChong96 JasonChong96 commented Jul 20, 2020

Problem

Links with no description should be excluded from search as some links should not be discoverable.

Solution

Add a condition to search to exclude urls with blank descriptions

Improvements:

  • Index will be managed by sequelize instead
  • Explicit configuration of search weights allow easy reconfiguration either through config.ts or env vars
  • Empty state graphic should no longer overlap the sort panel
  • Nav buttons hidden on search page

Deploy notes:

  • Run migration script before deploying on production. This will allow sequelize to create the new updated search index in place of the old one.

@liangyuanruo
Copy link
Contributor

Code lgtm, but one nagging doubt I have is the effect of deploying this to the database during a period of high search usage. It's generally considered bad practice to create indexes on deployment, but the performance impact is probably acceptable give that it takes only ~1s.

Maybe @LoneRifle can advise as well.

@LoneRifle
Copy link
Contributor

You really really should be letting sequelize handle this for you:
https://sequelize.org/master/manual/indexes.html

@JasonChong96
Copy link
Contributor Author

You really really should be letting sequelize handle this for you:
https://sequelize.org/master/manual/indexes.html

Will check if this is possible for the search index

@LoneRifle
Copy link
Contributor

What's the intention actually? Do you wish to keep the index schema fresher than what sequelize.sync tries to do? Bear in mind that because sequelize.sync uses CREATE X IF NOT EXISTS, it tends to refrain from altering existing db objects

@JasonChong96
Copy link
Contributor Author

It was initially done with a sql migration script as sequelize doesn't have support for postgres full text search so I didn't think it would be possible to use sequelize's sync for a text search index but that might not be the case

@JasonChong96
Copy link
Contributor Author

JasonChong96 commented Jul 20, 2020

Changed to use sequelize to manage index. Migration script has to be run to drop the previously manually managed index on production before deployment

@JasonChong96 JasonChong96 changed the title feat: auto re-index and exclude blank description from search feat: index tweaks and exclude blank description from search Jul 20, 2020
@JasonChong96 JasonChong96 merged commit 5c24975 into develop Jul 20, 2020
@JasonChong96 JasonChong96 deleted the search-indexing branch July 20, 2020 12:33
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.

3 participants