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

[DataQuery] New DQT backend implementation #8268

Merged
merged 27 commits into from
Aug 30, 2023
Merged

Conversation

driusan
Copy link
Collaborator

@driusan driusan commented Dec 13, 2022

This is intended to make it easier to review/test #8219 or #8216 by implementing the swagger schema, so that it can be tested via the the api docs module.

It's blocked by #8221 and #8260. In order to have anything to query against, you probably also need #8216 (or another QueryEngine implementation for a module.)

Nonetheless, the implementation should be complete (other than the one endpoint described as aspirational in the swagger schema.) and can be reviewed. I may have broken something while trying to extract it from the branch where I was developing the frontend or while trying to appease phpcs/phan.

@driusan driusan added State: Blocked PR or issue awaiting an external event such as the merge or another PR to proceed Blocking PR should be prioritized because it is blocking the progress of another task Language: PHP PR or issue that update PHP code labels Dec 13, 2022
@driusan driusan force-pushed the DQTRestBackend branch 2 times, most recently from 53bf5fc to a0d5de6 Compare December 16, 2022 19:28
@driusan driusan added the State: Needs work PR awaiting additional work by the author to proceed label Dec 19, 2022
@driusan
Copy link
Collaborator Author

driusan commented Dec 19, 2022

Needs work, not in sync with the latest updates to #8219

@driusan driusan removed the State: Needs work PR awaiting additional work by the author to proceed label Dec 19, 2022
@driusan driusan force-pushed the DQTRestBackend branch 2 times, most recently from 7032e29 to bc6858a Compare December 19, 2022 18:52
@driusan driusan added Blocking PR should be prioritized because it is blocking the progress of another task and removed Blocking PR should be prioritized because it is blocking the progress of another task State: Blocked PR or issue awaiting an external event such as the merge or another PR to proceed labels Jan 24, 2023
@driusan driusan added the Priority: High PR or issue should be prioritised over others for review and testing label Aug 8, 2023
@xlecours
Copy link
Contributor

I have sent the proposed data to POST /dataquery/queries and got a 200 {"QueryID": 1}
But then, using GET /dataquery/queries nothing shows

@driusan
Copy link
Collaborator Author

driusan commented Aug 17, 2023

I believe the sample data in the schema.yml assumes that the candidate_parameter's QueryEngine is implemented (Done in PR#8288, as of now unmerged). It should have returned a 400 error. It looks like it accepted it because of this FIXME needing to be implemented: https://github.com/aces/Loris/pull/8268/files#diff-d21396483bc372130220b07b323e521f63ff1aca56372eb09102c3b4c19e324aR65

Copy link
Contributor

@xlecours xlecours left a comment

Choose a reason for hiding this comment

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

This makes sense. It's hard to test until there is a frontend to it.

modules/dataquery/php/queries.class.inc Outdated Show resolved Hide resolved
@driusan
Copy link
Collaborator Author

driusan commented Aug 17, 2023

TODO: After this PR is merged make an issue about instruments module needing to separate viewing data permission from data_entry. (Currently, the instruments module does has a hasPermission('data_entry') permission check. That means that you need data_entry to query the module data in the dataquery module.)

Copy link
Contributor

@zaliqarosli zaliqarosli 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! the only issue I had is the one already added by dave as a to-do above ^^

@driusan
Copy link
Collaborator Author

driusan commented Aug 21, 2023

@zaliqarosli @xlecours re-factored code as per Xavier's request, can you guys re-review?

Copy link
Contributor

@xlecours xlecours left a comment

Choose a reason for hiding this comment

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

Let's roll :)

@driusan driusan merged commit 0ae7a24 into aces:main Aug 30, 2023
19 checks passed
@ridz1208 ridz1208 added this to the 26.0.0 milestone Nov 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Blocking PR should be prioritized because it is blocking the progress of another task Language: PHP PR or issue that update PHP code Priority: High PR or issue should be prioritised over others for review and testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants