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

[JN-1461] reference other study survey answers in search expressions #1166

Open
wants to merge 9 commits into
base: development
Choose a base branch
from

Conversation

connorlbark
Copy link
Collaborator

@connorlbark connorlbark commented Oct 28, 2024

DESCRIPTION (include screenshots, and mobile screenshots for participant UX)

Thought process: We keep search expressions study specific but certain terms can optionally be study-switchable. For example, {answer.my_survey.diagnosis = 'something'} and {answer["my_other_study"].my_other_survey.diagnosis} = 'something else'. This means "look for any enrollee with the same profile id as the enrollee in this study and enrolled in "my_other_study" that has a response to this question". This keeps the search expressions (imo) pretty intuitive. Currently, answer is the only term that's study-switchable, but other ones could be, too. They just need to add parse(studyname, variable) to their search term parser.

The UI is totally unaware of this new syntax, so you have to use the advanced editor to use it. In the future, we can probably figure out a way to get our facets to also look at other studies in the same portal environment.

TO TEST: (simple manual steps for confirming core behavior -- used for pre-release checks)

  • observe tests

@connorlbark connorlbark changed the base branch from development to cb-members-choice-simplify-search-exp-infra October 28, 2024 18:33
@connorlbark connorlbark changed the title [member's choice] reference other study survey answers [member's choice] reference other study survey answers in search expressions Oct 28, 2024
Base automatically changed from cb-members-choice-simplify-search-exp-infra to development October 31, 2024 18:59
@connorlbark connorlbark changed the title [member's choice] reference other study survey answers in search expressions [JN-1461] reference other study survey answers in search expressions Oct 31, 2024
@connorlbark connorlbark marked this pull request as ready for review October 31, 2024 19:41
Copy link
Collaborator

@devonbush devonbush left a comment

Choose a reason for hiding this comment

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

looks good, some codesmithing suggestions.


public Optional<Answer> findByProfileIdByStudyAndQuestion(UUID profileId, String studyName, String surveyStableId, String questionStableId) {
return jdbi.withHandle(handle ->
handle.createQuery("select a.* from " + tableName + " a " +
Copy link
Collaborator

Choose a reason for hiding this comment

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

please use multiline """ string and .formatted

@@ -79,4 +79,24 @@ public Optional<Answer> findForEnrolleeByQuestion(UUID enrolleeID, String survey
.findFirst()
);
}

public Optional<Answer> findByProfileIdByStudyAndQuestion(UUID profileId, String studyName, String surveyStableId, String questionStableId) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is a tough one to name... but the two 'bys' threw me off

Suggested change
public Optional<Answer> findByProfileIdByStudyAndQuestion(UUID profileId, String studyName, String surveyStableId, String questionStableId) {
public Optional<Answer> findByProfileIdStudyAndQuestion(UUID profileId, String studyName, String surveyStableId, String questionStableId) {

String modelName = splitModelName.get(0);
String studyName = null;

if (modelName.contains("[")) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this feels like parser logic that belongs more in the antlr parser. Is it possible to have a VARIABLE_WITH_STUDY term? or otherwise update our parsing so that the syntax for how studies are specified is handled there?

Copy link
Collaborator Author

@connorlbark connorlbark Nov 5, 2024

Choose a reason for hiding this comment

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

I totally agree and I have ideas on how to do this but my antlr skills are not nearly refined enough to accomplish them. I tried doing

expr:  term OPERATOR term | expr AND expr | expr OR expr | NOT expr | INCLUDE PAR_OPEN term PAR_CLOSE | PAR_OPEN expr PAR_CLOSE;
term: value | function | variable;
value: NUMBER | STRING  | BOOLEAN | NULL;
function: FUNCTION_NAME PAR_OPEN term (',' term)* PAR_CLOSE;
variable: VAR_OPEN VAR_MODEL (VAR_STUDY)? (VAR_ARGUMENTS)? VAR_CLOSE;

// Lexer rules
NUMBER: [0-9]+ ('.' [0-9]+)?;
STRING: '\'' (~[\\'\r\n])* '\'';
VAR_STUDY: '["'[a-zA-Z0-9_]+'"]';
VAR_MODEL: [a-zA-Z0-9_]+;
VAR_ARGUMENTS: '.'([a-zA-Z0-9_]|'.')+;
VAR_SEPARATOR: '.';
VAR_OPEN: '{';
VAR_CLOSE: '}';

But there's some weird execution order issues where it's trying to read functions as variables even though there isn't a {} surrounding them. I can't figure out why... I might punt this off as a future improvement.

Copy link

sonarcloud bot commented Nov 5, 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.

3 participants