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

Jt/visually segmented case search groups #1511

Merged
merged 13 commits into from
Dec 13, 2023

Conversation

Jtang-1
Copy link
Contributor

@Jtang-1 Jtang-1 commented Dec 5, 2023

Product Description

This change affects queryResponse. It adds groupKey to display elements and groupHeaders dictionary to queryResponse. The groupKey of each display element corresponds to a key in groupHeaders.

Technical Summary

Tech Spec
Jira Ticket
Research Doc

Commcare-core PR: dimagi/commcare-core#1389
HQ PR: dimagi/commcare-hq#33828

Safety Assurance

Safety story

locally tested.
This does not alter existing response values but only adds new key:values.

Automated test coverage

Will add tests

QA Plan

Will QA

Special deploy instructions

Needs to be merged with/after dimagi/commcare-core#1389

Rollback instructions

  • This PR can be reverted after deploy with no further considerations.

Review

  • The set of people pinged as reviewers is appropriate for the level of risk of the change.

Copy link

codecov bot commented Dec 5, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (56fe7e0) 69.93% compared to head (519223b) 69.90%.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #1511      +/-   ##
============================================
- Coverage     69.93%   69.90%   -0.03%     
- Complexity     1958     1959       +1     
============================================
  Files           250      250              
  Lines          7649     7656       +7     
  Branches        692      692              
============================================
+ Hits           5349     5352       +3     
- Misses         2037     2043       +6     
+ Partials        263      261       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Jtang-1 Jtang-1 requested review from shubham1g5 and nospame December 5, 2023 17:28
@Jtang-1 Jtang-1 added product/feature-flag Change will only affect users who have a specific feature flag enabled cross requested If a PR is dependent on commcare-core work labels Dec 5, 2023
@Jtang-1 Jtang-1 marked this pull request as ready for review December 8, 2023 00:39
Copy link
Contributor

@nospame nospame left a comment

Choose a reason for hiding this comment

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

Reviewed with Robert, looks good overall

Copy link
Contributor

@shubham1g5 shubham1g5 left a comment

Choose a reason for hiding this comment

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

Couple Minor comments.

In Addition, we should update the libs/commcare to point to the head of FP branch in this PR before you merge it (after merging the corresponding core PR) so that we don't overwrite any changes that got to libs/commcare after you update the submodule. The practice I usually follow here is to not update the submodule at all in the formplayer PR until I am about to merge it. When the Prs are ready to be merged I do the following sequence of events -

  1. Merge commcare-core PR into formplayer branch
  2. Update submodule in Formplayer PR to point to head of formplayer branch in commcare-core
  3. Wait for tests to pass in formplayer PR and merge if the build passes.

Comment on lines 95 to 102
OrderedHashtable<String, QueryGroup> queryGroupMap = queryScreen.getGroupHeaders();
groupHeader = new OrderedHashtable<>();
for (Map.Entry<String, QueryGroup> entry : queryGroupMap.entrySet()) {
String key = entry.getKey();
QueryGroup queryGroupItem = entry.getValue();
String text = queryGroupItem.getDisplay().getText().evaluate(ec);
groupHeader.put(key, text);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

would be nice to contain this logic in a query screen method itself like queryScreen.evalGroupHeaders()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

11d6ed6 and 15a0d9e

Can you explain what the responsibility of queryScreen is? Why would the preference be to put the logic in there instead of extracting it to another function in queryResponseBean? I would imagine queryResponesBean is responsible for generating the data in the response.

Copy link
Contributor

Choose a reason for hiding this comment

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

queryResponseBean is supposed to me more of a response model wrapper class over screen classes with screen classes being the classes that contain any evaluation logic. We don't follow it strictly but that's the idea atleast in theory.

Copy link
Contributor

@shubham1g5 shubham1g5 left a comment

Choose a reason for hiding this comment

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

Approved (but core submodule need to be updated before merge)

@Jtang-1 Jtang-1 merged commit eedb907 into master Dec 13, 2023
5 of 6 checks passed
@Jtang-1 Jtang-1 deleted the jt/visually-segmented-case-search-groups branch December 13, 2023 20:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cross requested If a PR is dependent on commcare-core work product/feature-flag Change will only affect users who have a specific feature flag enabled
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants