Skip to content

Conversation

@ridz1208
Copy link
Collaborator

@ridz1208 ridz1208 commented May 9, 2025

Brief summary of changes

When declared as single the new DQT does not allow to choose the option "is empty" in the filter. since this trait is used for all legacy instruments, it seems safer to use optional than SINGLE.

Testing instructions (if applicable)

Link(s) to related issue(s)

  • Resolves # (Reference the issue this fixes, if any.)

@skarya22
Copy link
Contributor

@ridz1208 I am not quite sure how to test this .. Where is the _getDataDictionaryGroup function used? It doesn't say anywhere when I search it in the repo

@ridz1208
Copy link
Collaborator Author

@skarya22 ideally you go to the new DQT, if you try to add a filter for an instrument field, with my changes you will get the isEmpty option, without my changes there is only the base operators

@skarya22
Copy link
Contributor

Works well for AOSI (PHP Instrument)
Before:
image
After:
image

But BMI Calculator still has no Empty options (LINST instrument)
image

Do we want to fix for LINST too or nah

@skarya22 skarya22 added the State: Needs clarification PR or issue that needs to clarify its intent to proceed label May 22, 2025
@ridz1208
Copy link
Collaborator Author

ridz1208 commented Aug 11, 2025

@skarya22 can you clarify what needs clarification ?

what do you mean by fix for LINST? there should not be a difference between LINST and PHP in terms of dictionary

@skarya22
Copy link
Contributor

@skarya22 can you clarify what needs clarification ?

what do you mean by fix for LINST? there should not be a difference between LINST and PHP in terms of dictionary

BMI Calculator still has no Empty options, if that is not intentional then I think the PR needs work for LINST to have the same options

@skarya22 skarya22 added State: Needs work PR awaiting additional work by the author to proceed and removed State: Needs clarification PR or issue that needs to clarify its intent to proceed labels Aug 12, 2025
@ridz1208
Copy link
Collaborator Author

@skarya22 Added the OPTIONAL for all cases I could think of that could potentially be "empty"

@ridz1208 ridz1208 added Module: datadict PR or issue related to (old) datadict module Module: dataquery PR or issue related to (new) dataquery module Language: PHP PR or issue that update PHP code and removed State: Needs work PR awaiting additional work by the author to proceed labels Oct 16, 2025
@ridz1208 ridz1208 force-pushed the instrument_legacy_dictionary branch from f95c565 to e49335b Compare October 17, 2025 13:12
@ridz1208
Copy link
Collaborator Author

@kongtiaowang this PR keeps failing on "waiting for mysqld" and timing out but I just rebased it and it doesnt touch on any SQL code. is 26.0-release broken ?

@kongtiaowang
Copy link
Contributor

After [github CI] waiting for mysqld - fix in 26.0-release #10118 get merged will fix the issue.

@driusan
Copy link
Collaborator

driusan commented Nov 11, 2025

#10118 is merged

@ridz1208
Copy link
Collaborator Author

FIANLLY. Thanks @kongtiaowang

@ridz1208 ridz1208 removed their assignment Nov 11, 2025
@driusan driusan merged commit a144236 into aces:26.0-release Nov 12, 2025
80 of 140 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Language: PHP PR or issue that update PHP code Module: datadict PR or issue related to (old) datadict module Module: dataquery PR or issue related to (new) dataquery module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants