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(#9293): refactor freetext search views #9308

Closed
wants to merge 15 commits into from

Conversation

m5r
Copy link
Member

@m5r m5r commented Aug 8, 2024

Description

#9293

Code review checklist

  • Readable: Concise, well named, follows the style guide, documented if necessary.
  • Documented: Configuration and user documentation on cht-docs
  • Tested: Unit and/or e2e where appropriate
  • Internationalised: All user facing text
  • 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.

@m5r m5r force-pushed the 9293-research-freetext-views branch from 91c69a7 to 6fd1930 Compare September 11, 2024 12:29
@m5r m5r requested a review from jkuester September 12, 2024 18:17
@m5r
Copy link
Member Author

m5r commented Sep 12, 2024

@jkuester could you give this a brief look when you have time? I made some changes in the view to simplify the code (like iterating over the include keys instead of each document object keys) and I'd like a sanity check to make sure the numbers I shared in the issue are coherent

Copy link
Contributor

@jkuester jkuester left a comment

Choose a reason for hiding this comment

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

Sorry I missed this last week!

});

if (doc.contact && doc.contact._id) {
emitMaybe('contact:' + doc.contact._id.toLowerCase(), doc.reported_date);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is an interesting case I had not noticed before.... 🤔 Looks like this is to support the ancient ANC workflow Gareth mentioned here. I cannot see any place this is currently used in the code (but after my adventures with the case_id, who knows...).

I guess maybe it is safer to leave it... 🤷

@m5r m5r force-pushed the 9293-research-freetext-views branch 2 times, most recently from 8998dee to a639ace Compare September 17, 2024 14:52
@m5r m5r changed the title DO NOT MERGE - Research reducing freetext views' size feat(#9293): refactor freetext search views Sep 19, 2024
@m5r m5r linked an issue Sep 19, 2024 that may be closed by this pull request
4 tasks
@m5r m5r force-pushed the 9293-research-freetext-views branch from 06976a0 to 1d6abd2 Compare September 23, 2024 13:46
@m5r m5r marked this pull request as ready for review September 23, 2024 13:48
@m5r
Copy link
Member Author

m5r commented Sep 23, 2024

I rebased the branch off master to fix this weird CI issue but I kept a snapshot of the branch rebased off 4.10.x in the branch 9293-freetext-views-4.10

@m5r m5r closed this Oct 15, 2024
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.

Research freetext view use to inform hosting TCO reduction
2 participants