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

Refactoring Add script to count unique learners in Sensei #2178

Merged
merged 2 commits into from
Jan 31, 2024

Conversation

jonathanbossenger
Copy link
Collaborator

@jonathanbossenger jonathanbossenger commented Jan 26, 2024

Update to #2172
Fixes #2111
Add sanitization for query strings
Use wpdb->prepare for SQL query

Add sanitization for query strings
Use wpdb->prepare for SQL query
@jonathanbossenger
Copy link
Collaborator Author

@bsanevans In attempting to refactor #2172 to include sanitizing the query variables and to use $wpdb->prepare for the query, I had to make quite a few changes to the code.

Could I ask you to get @m1r0 to review this refactored version to confirm it would still work functionally the same as the original code, then I'll get someone else from the meta team to review it.

@kaitohm
Copy link
Contributor

kaitohm commented Jan 27, 2024

I've reached out to @m1r0 .

Copy link

@m1r0 m1r0 left a comment

Choose a reason for hiding this comment

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

Hello, hello! 👋 🙂

still work functionally the same as the original code

The only functional difference I'm noticing is that now both dates are needed. If that's intended, I've left some feedback.

Everything else looks solid. 🪨

wp-content/plugins/wporg-learn/inc/sensei.php Outdated Show resolved Hide resolved
@kaitohm
Copy link
Contributor

kaitohm commented Jan 30, 2024

It would be great if we didn't need to set both dates. For example, if we could get all data from a specific date until today without needing to set today's date, that would be great.

@jonathanbossenger Is that a change you can implement?

Suggested improvement from @m1r0
@jonathanbossenger
Copy link
Collaborator Author

Thanks @m1r0 and @bsanevans I've updated the PR.

@pkevan This PR is an update to #2172, adding sanitization to the date variables, and changing the SQL concatenation to use a prepared statement.

Could we ask you to give this a review?

Thanks

Copy link
Contributor

@pkevan pkevan 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

@jonathanbossenger jonathanbossenger merged commit 297b0da into trunk Jan 31, 2024
1 check passed
@jonathanbossenger jonathanbossenger deleted the 2111-count-sensei-learners branch January 31, 2024 10:22
bazza pushed a commit to WordPress/wordpress.org that referenced this pull request Jan 31, 2024
Github should now be at `433848db54f76e1c5fd065167b1e8cd30ba53d34`


git-svn-id: https://meta.svn.wordpress.org/sites/trunk@13160 74240141-8908-4e6f-9713-ba540dce6ec7
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.

Script to count unique learners
4 participants