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 10 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
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.

Comment on lines +99 to +100
await initialReplicationLib.replicate(remoteDb, localDb);
if (await initialReplicationLib.isReplicationNeeded(localDb, userCtx)) {
Copy link
Member

Choose a reason for hiding this comment

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

question: I might be missing something here but is this doing replication first then checking if replication is needed?

['health_center', 1],
['clinic', 2],
['person', 3],
].forEach(([type, typeIndex]) => it('emits numerical index for default type', () => {
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: it would be better to also put(string interpolate) type value into the test title as it would give us an idea as to what test passed and what failed faster.
Right now it looks like this:
image

['contact', 2, 'clinic'],
['contact', 3, 'person']
].forEach(([type, typeIndex, contactType]) => it(
'emits numerical index for default type when used as custom type',
Copy link
Member

Choose a reason for hiding this comment

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

same comment as above


after(commonPage.logout);

it('search by NON empty string should display results with contains match and clears search', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
it('search by NON empty string should display results with contains match and clears search', async () => {
it('search by NON empty string should display results which contains match and clears search', async () => {

Comment on lines +14 to +20
const sittuHealthCenter = placeFactory.place().build({
name: 'Sittu Health Center',
type: 'health_center',
parent: { _id: districtHospitalId, parent: { _id: '' } }
});

const potuHospital = placeFactory.place().build({
name: 'Potu Hospital',
type: 'district_hospital',
parent: { _id: '', parent: { _id: '' } }
const potuHealthCenter = placeFactory.place().build({
Copy link
Member

Choose a reason for hiding this comment

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

question: what's a sittu and potu?

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"
2 participants