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

feat: add where function to dt-reports class #2484

Merged
merged 5 commits into from
Jun 27, 2024

Conversation

squigglybob
Copy link
Collaborator

Adds functionality to the class in dt-reports.php to find reports where some things are true.

@squigglybob
Copy link
Collaborator Author

@corsacca I've added in this function so I can pull out reports that only match on certain fields.

I'm thinking actually that this could be more powerful if it mimicked something like laravels where function where you can add in comparison functions.

so instead of an assosciative array which assumes equality in the where clause, it would have an array of arrays

Disciple_Tools_Reports::where( [
   [ 'column_name', '=', $value ],
] );

and the middle value of the array could also be '!=', '>', '<', 'not' etc.

dt-reports/reports.php Outdated Show resolved Hide resolved
@squigglybob
Copy link
Collaborator Author

squigglybob commented May 22, 2024

Are there any other changes that need to be made in light of our recent conversation about keeeping things consistent? @corsacca

@corsacca
Copy link
Member

hey @squigglybob

Disciple_Tools_Reports::where( [
   [ 'column_name', '=', $value ],
] );

This does not seem to match the code.

We also talked about matching the format in https://developers.disciple.tools/theme-core/api-posts/list-query

@squigglybob
Copy link
Collaborator Author

hey @squigglybob

Disciple_Tools_Reports::where( [
   [ 'column_name', '=', $value ],
] );

That was an example of how I was thinking it could be. It's not currently how it works.

This does not seem to match the code.

We also talked about matching the format in https://developers.disciple.tools/theme-core/api-posts/list-query

That was the thing I was thinking of. I'll look at getting it matching that format next

@squigglybob
Copy link
Collaborator Author

@corsacca hey bro, when you're back in the saddle, it would be good to connect on this one.

Currently the where function matches the list_posts search and query for the very simple cases (except for putting the values inside an array, which could be coded to cope with string/array depending on what result is desired)

could we merge this one in, and I can create a new ticket to upgrade the where clause to have more of the complicated features like ORing etc.

The reports table and posts are quite different in the way they work, so it would be good to talk through exactly how we want the search query params to work.

@corsacca
Copy link
Member

@squigglybob

The function name where seems a bit confusion. Maybe we use: list_reports() instead.

The current format for the $args or $where is:

[ 'post_id' => 3, 'type' => 'key' ]

@todo, add a comment for expected args format

This format does not support the expansion of the operations ( '!=', '>', '<', 'not' etc. )
So when/if we do want to add them we'd need to refactor the args and change the places using the function across multiple plugins.

I'd rather we build this out nicely. We can use direct $wpdb calls until it is done, which might be simpler anyways.

…list

putting the value in an array will mean we can add operators and other upgrades to how the where works later
@squigglybob
Copy link
Collaborator Author

@corsacca I should have implemented what we talked about straight away. 😬 Hopefully I remembered what we talked about
Should be done if i did 😁

@corsacca
Copy link
Member

Thanks @squigglybob!

@corsacca corsacca merged commit a8ac58a into develop Jun 27, 2024
4 checks passed
@corsacca corsacca deleted the feat/dt-reports/added-functionality branch June 27, 2024 10:48
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.

None yet

2 participants