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(#9544): add offline freetext search indexes #9661

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

Conversation

jkuester
Copy link
Contributor

@jkuester jkuester commented Nov 22, 2024

Description

Closes #9544

Code review checklist

  • Readable: Concise, well named, follows the style guide, documented if necessary.
  • Tested: Unit and/or e2e where appropriate
  • Backwards compatible: Works with existing data and configuration or includes a migration. Any breaking changes documented in the release notes.

Compose URLs

If Build CI hasn't passed, these may 404:

License

The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.

@jkuester jkuester changed the title 9544 offline freetext feat(#9544); add offline freetext search indexes Nov 22, 2024
@jkuester jkuester changed the title feat(#9544); add offline freetext search indexes feat(#9544): add offline freetext search indexes Nov 22, 2024
initialReplicationLib.isReplicationNeeded(localDb, userCtx),
swRegistration,
setReplicationId(POUCHDB_OPTIONS, localDb),
offlineDdocs.init(localDb)
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 is the only real change here. The rest is just a quick re-factor to use async/await.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These changes in shared-libs/search are just temporary. The proper changes will need to be made against the cht-datasource code once we re-base on top of that.

This is why I have not added any additional unit tests to cover this logic (or worried too much about code structure).


describe('contacts_by_freetext', () => {
[
['online view', loadView('medic-db', 'medic-client', 'contacts_by_freetext')],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We will need to remove this once the medic-client/contacts_by_freetext view is deleted, but for now I thought it was useful to keep this in here to prove that the new offline indexes behave exactly the same as the original ones.

@jkuester jkuester requested a review from sugat009 November 27, 2024 21:12
@jkuester
Copy link
Contributor Author

c.c. @m5r and @tatilepizs Particularly regarding how I have updated the e2e search tests to run both for an online and an offline user. I think this means these same tests can cover the offline indexes and the new Lucene functionality. 👍

@jkuester jkuester marked this pull request as ready for review November 27, 2024 21:14
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 bootstrapper code gets run during the main initialization of the app (before any of the Angular code spins up. This bootstrapping gets run during initial login and also when reloading the app (e.g. when the webapp-code/app-settings/ddocs change).

So, any time in the future when we make updates/additions/removals of views here, we can be confident that everything will be initialized properly in the local db when the app reloads.

Copy link
Contributor

@m5r m5r left a comment

Choose a reason for hiding this comment

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

Nothing to add to Sugat's comments. I left an open-ended question about warming up views in the bootstrapper but it's not a blocker

@jkuester jkuester requested a review from sugat009 January 6, 2025 22:45
@jkuester
Copy link
Contributor Author

jkuester commented Jan 8, 2025

For clarity, this PR is now is now blocked waiting on #9541. They should both be merged together before going to master. We cannot ship this one earlier because it would involve adding new offline indexes without actually removing the existing indexes that exist both online and offline. The effect would be to double the amount of space used on offline devices by the freetext indexes....

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.

Make freetext search view indexes "offline only"
3 participants