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

use instance_id_ variables For Multi Select Endpoints #1458

Closed
wants to merge 1 commit into from

Conversation

shubham1g5
Copy link
Contributor

@shubham1g5 shubham1g5 commented Sep 6, 2023

Technical Summary

This is a modification on top of the multi-select session endpoints support. Instead of substituting the instance arguments directly, use a prefix instance_id_argname to signify that we want to substitue the argument with corresponding instance guid.

Safety Assurance

Safety story

Automated test coverage

QA Plan

Migrations

  • The migrations in this code can be safely applied first independently of the code.

Special deploy instructions

  • This PR can be deployed after merge with no further considerations.

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.

cross-request: dimagi/commcare-core#1327

@shubham1g5
Copy link
Contributor Author

shubham1g5 commented Sep 6, 2023

@snopoke This needs cleanup but I kind of just want to get a OK on the approach here before making further changes here and on HQ. This concern came up whille working on inline case search changes which might need us to retain both the original argument value along with the substituted instance argument and I would prefer to keep the actual argument name as it is.

I confirmed with delivery that this is not deployed yet in any of their apps so should not need backward compatibility.

Copy link
Contributor

@snopoke snopoke left a comment

Choose a reason for hiding this comment

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

I don't really like this approach because it depends on some internal naming convention in the engine.

I made this comment on the spec as well but could not use the instance to get the list of case IDs in the query:

<data key="case_id" nodeset="instance('selected_cases')/results/value" ref="."/>

@shubham1g5 shubham1g5 closed this Mar 18, 2024
@shubham1g5 shubham1g5 deleted the endpointInstanceVariable branch March 18, 2024 19:26
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.

2 participants