Refactor students page fetching data through Gutenberg and avoiding subqueries #5104

renatho · 2022-05-09T10:02:54Z

Changes proposed in this Pull Request

It removes the subquery from the students query in order to make it work in environments with optimizations where the tables can live in different places.
It removes an enqueued script that wasn't being used.
It refactors the courses fetch in order to optimize and simplify using the useSelect.
It introduces a new hook useSelectWithDebounce in order to expand the useSelect, to work with debounce.
It removes a middleware we were using to change our rest API arguments to query strings. It seems it's working properly without the middleware with the URL rewrite enabled or not.
It tweaks the wp_kses we use for the Students table, in order to support it in different envs that have customization on the wp_kses, like removing the data-*.

Testing instructions

Create some courses and some students.
Make some progress with some users.
Visit WP admin > Sensei LMS > Students, and make sure it's listed properly as previously, especially the "Last Activity" column.
Add and remove a student from a course, and make sure it works as previously.

m1r0

Looks good to me and works well. 👍

renatho · 2022-05-10T09:17:23Z

@m1r0, thank you for your review! But I realized that there are some other issues. I'll try to fix all related issues in this same PR.
I also added a new commit now to remove an enqueue script that we weren't using: 3397c4a

…ents-subquery

renatho · 2022-05-10T16:15:53Z

Changing to draft again since I need to fix one other related thing.

gabrielcaires

Thanks @renatho for this refactoring, I was already planning to refactor it to support new features.

Could you please explain how adding a wordpress/data avoid subqueries?
One of our reasons to not use wordpress/data was because the sensei wordpress/data version is pretty outdated (2 years ago) so are avoiding to create new features that doesn't require a strong state management until we update it.

Do you think it is really necessary to solve the problem?

I saw you are removing nock, usually mock the fetch @wordpress/data library is not a good practice because it creates a coupling between the test and the implementation.
It already helped us to get some issues.

…ings

gabrielcaires

As we discussed, the main motivation of this PR on the frontends ide is to remove the http-client.

I would suggest remove the changes related to ~@wordpress/data because we are using @wordpress/apiFetch that is also a standard way to load data from wordpress.
it avoid us to use a store and all related infrastructure to support wordpress/data.

What do you think?

renatho · 2022-05-12T01:49:18Z

Could you please explain how adding a wordpress/data avoid subqueries?

They are unrelated changes. I uptated the PR title in order to make it more clear. 😉

One of our reasons to not use wordpress/data was because the sensei wordpress/data version is pretty outdated (2 years ago) so are avoiding to create new features that doesn't require a strong state management until we update it.

Just registering here in the PR, as we talked, the version we have installed is for the unit tests only. When running the site, it will import it from from the Gutenberg bundled version in WordPress core: https://developer.wordpress.org/block-editor/contributors/versions-in-wordpress/. More details in: https://www.npmjs.com/package/@wordpress/dependency-extraction-webpack-plugin

I would suggest remove the changes related to ~@wordpress/data because we are using @wordpress/apiFetch that is also a standard way to load data from wordpress.
it avoid us to use a store and all related infrastructure to support wordpress/data.

I was thinking a little about that. I think the only benefit of not using wordpress/data is not including their dependencies in the student page, but it's not a big deal since we're in wp-admin and it's used in many other pages, caching it separately. And using the useSelect there with the store, it brings many benefits, like using an implementation that is specifically for this purpose, saving some code writing and being more prepared for future changes.

So I'd say that it would be nice to continue with wordpress/data, unless you have a strong reason to not use it. WDYT?

gabrielcaires · 2022-05-12T12:11:51Z

assets/react-hooks/use-select-with-debounce.js

+import { useSelect } from '@wordpress/data';
+import { useState, useEffect, useCallback } from '@wordpress/element';
+
+// TODO: Write tests.


Are you going to add this test?

A partial test added here: 859125c, but I couldn't make the debounce work yet. So I'll keep this part as TODO for now. 😉

gabrielcaires · 2022-05-12T12:14:03Z

assets/admin/students/student-modal/index.test.js

+/**
+ * WordPress dependencies
+ */
+import { useSelect } from '@wordpress/data';


Suggestion, replace useSelect to useEntityRecords
const { records, isResolving } = useEntityRecords( 'postType', 'courses' );

Thank you for the suggestion! It would be nice, but this hook was exposed recently, so we can't use that yet in order to support older versions. See it in: WordPress/gutenberg@8b8886c

gabrielcaires

@renatho I still think that configuring a store just to render a list inside a modal (it is not a SPA) just to make a get a bit overkill but considering it was moved to a hook if we can easy switch if we saw some issue with this approach.

I just noticed your add a TODO to write the hook test. Are you planning add this test?

The test is not complete, since we're having an issue to simulate the debounce timers.

aaronfc

Looks good and works well 👍

aaronfc · 2022-05-16T09:21:34Z

assets/react-hooks/use-select-with-debounce.test.js

+		// TODO: Complete test running debounce to make sure it was called after the time.
+		// jest.runAllTimers();
+		// expect( mockFn ).toBeCalledWith( mapSelect, deps2 );


Do you plan to do this before merging?

Unfortunately, I don't plan to add it now. We're having some problems with dependencies in order to make it work. runAllTimers is starting an infinite loop in the debounce. There are some issues about that on the Internet and it was supposed to be fixed, so at some point, we need to dig into the dependencies to see what's happening.

Just adding some links here for future reference about this:

jestjs/jest#3465
lodash/lodash#2893
jestjs/jest#7776

aaronfc · 2022-05-16T09:30:43Z

includes/class-sensei-db-query-learners.php

+		// phpcs:disable WordPress.DB.DirectDatabaseQuery.NoCaching, WordPress.DB.DirectDatabaseQuery.DirectQuery
+		$results = $wpdb->get_results(
+			// phpcs:disable WordPress.DB.PreparedSQL.InterpolatedNotPrepared, WordPress.DB.PreparedSQLPlaceholders.UnfinishedPrepare -- Placeholders created dinamically.


Just writing to confirm that you are using phpcs:disable on purpose. As opposed to phpcs:ignore this will disable the check until the end of the current file not just on the following line.

Good catch! It wasn't on purpose. Fixed in: 4e602d0

renatho · 2022-05-16T12:27:09Z

After the last approval, I just added a commit fixing PHPCS comments: 4e602d0. We need this PR in the next release, so considering this change is simple and doesn't impact how the plugin work, I'm going ahead and merging this. Please, let me know if there's something else on that, so I can raise a new PR. 😉

Separate learners subquery

1421b76

renatho added the [Type] Bug label May 9, 2022

renatho added this to the 4.4.1 milestone May 9, 2022

renatho requested review from a team May 9, 2022 10:02

renatho self-assigned this May 9, 2022

Fix PHPCS

5c93235

m1r0 previously approved these changes May 9, 2022

View reviewed changes

Remove unused enqueue

3397c4a

renatho marked this pull request as draft May 10, 2022 09:15

renatho dismissed m1r0’s stale review via 3397c4a May 10, 2022 09:16

renatho added 3 commits May 10, 2022 14:56

Add hook to handle useSelect with debounce

f610415

Refactor course list to use core store

b60e246

Merge branch 'trunk' of github.com:Automattic/sensei into change/stud…

2467160

…ents-subquery

renatho force-pushed the change/students-subquery branch from 0c9023d to 2467160 Compare May 10, 2022 14:04

renatho requested a review from gabrielcaires May 10, 2022 14:04

renatho changed the title ~~Separate learners subquery~~ Refactor students page in order to fetch data through Gutenberg and avoid subqueries May 10, 2022

renatho marked this pull request as ready for review May 10, 2022 14:11

renatho requested a review from m1r0 May 10, 2022 14:12

renatho marked this pull request as draft May 10, 2022 16:15

gabrielcaires reviewed May 10, 2022

View reviewed changes

Remove apiFetch middleware that changed the URL arguments to queryStr…

4fd0b31

…ings

renatho requested a review from gabrielcaires May 11, 2022 09:42

renatho marked this pull request as ready for review May 11, 2022 09:42

gabrielcaires requested changes May 11, 2022

View reviewed changes

Add div custom kses for specific envs

2e9c6bf

renatho changed the title ~~Refactor students page in order to fetch data through Gutenberg and avoid subqueries~~ Refactor students fetching data through Gutenberg and avoiding subqueries May 12, 2022

renatho changed the title ~~Refactor students fetching data through Gutenberg and avoiding subqueries~~ Refactor students page fetching data through Gutenberg and avoiding subqueries May 12, 2022

renatho requested a review from gabrielcaires May 12, 2022 01:49

gabrielcaires requested changes May 12, 2022

View reviewed changes

gabrielcaires previously approved these changes May 12, 2022

View reviewed changes

Add some tests to useSelectWithDebounce

859125c

The test is not complete, since we're having an issue to simulate the debounce timers.

renatho dismissed gabrielcaires’s stale review via 859125c May 13, 2022 21:12

renatho requested a review from gabrielcaires May 13, 2022 21:16

aaronfc previously approved these changes May 16, 2022

View reviewed changes

m1r0 modified the milestones: 4.4.1 - Report Improvements, 4.4.2 May 16, 2022

Fix PHPCS ignores

4e602d0

renatho dismissed aaronfc’s stale review via 4e602d0 May 16, 2022 12:20

renatho modified the milestones: 4.4.2, 4.4.1 - Report Improvements May 16, 2022

renatho merged commit 4bb212f into trunk May 16, 2022

renatho deleted the change/students-subquery branch May 16, 2022 12:40

mikeyarce mentioned this pull request Jul 31, 2022

Fix/last activity sort #5394

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor students page fetching data through Gutenberg and avoiding subqueries #5104

Refactor students page fetching data through Gutenberg and avoiding subqueries #5104

renatho commented May 9, 2022 •

edited

m1r0 left a comment

renatho commented May 10, 2022

renatho commented May 10, 2022

gabrielcaires left a comment

gabrielcaires left a comment

renatho commented May 12, 2022

gabrielcaires May 12, 2022

renatho May 13, 2022

gabrielcaires May 12, 2022 •

edited

renatho May 13, 2022

gabrielcaires left a comment

aaronfc left a comment

aaronfc May 16, 2022

renatho May 16, 2022

renatho May 16, 2022 •

edited

aaronfc May 16, 2022

renatho May 16, 2022

renatho commented May 16, 2022

Refactor students page fetching data through Gutenberg and avoiding subqueries #5104

Refactor students page fetching data through Gutenberg and avoiding subqueries #5104

Conversation

renatho commented May 9, 2022 • edited

Changes proposed in this Pull Request

Testing instructions

m1r0 left a comment

Choose a reason for hiding this comment

renatho commented May 10, 2022

renatho commented May 10, 2022

gabrielcaires left a comment

Choose a reason for hiding this comment

gabrielcaires left a comment

Choose a reason for hiding this comment

renatho commented May 12, 2022

gabrielcaires May 12, 2022

Choose a reason for hiding this comment

renatho May 13, 2022

Choose a reason for hiding this comment

gabrielcaires May 12, 2022 • edited

Choose a reason for hiding this comment

renatho May 13, 2022

Choose a reason for hiding this comment

gabrielcaires left a comment

Choose a reason for hiding this comment

aaronfc left a comment

Choose a reason for hiding this comment

aaronfc May 16, 2022

Choose a reason for hiding this comment

renatho May 16, 2022

Choose a reason for hiding this comment

renatho May 16, 2022 • edited

Choose a reason for hiding this comment

aaronfc May 16, 2022

Choose a reason for hiding this comment

renatho May 16, 2022

Choose a reason for hiding this comment

renatho commented May 16, 2022

renatho commented May 9, 2022 •

edited

gabrielcaires May 12, 2022 •

edited

renatho May 16, 2022 •

edited