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.

Sign up for GitHub

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

Jump to bottom

Extract get_courses with dependent methods from Sensei_Analysis_Overview_List_Table #4938

Merged
merged 32 commits into from Mar 30, 2022
Merged

Extract get_courses with dependent methods from Sensei_Analysis_Overview_List_Table #4938

merged 32 commits into from Mar 30, 2022

Conversation

merkushin
Copy link
Member

@merkushin merkushin commented Mar 22, 2022 •
edited

Proposed changes:

  • Add a list table factory and the list table for courses.
  • Add courses data provider.

Testing instructions:

  • Switch to trunk branch.
  • Open Reports and click "Courses" tab link.
  • Remember how it looks (I make screenshots for this purpose).
  • Switch to update/reports-structure-proposal.
  • Look for any changes. The page has to look the same.

In this PR, lessons and students list tables are not implemented, so you will get errors on corresponding pages.
These changes are part of:

@merkushin merkushin self-assigned this Mar 22, 2022
@donnapep
Copy link
Collaborator

Fetching data moved to data providers (bad name? :)).

Not sure it's entirely relevant, but we have something called enrolment providers (https://github.com/Automattic/sensei/blob/c1432e4e66f115e63fff50b4b434ca5eaf36d222/includes/enrolment/class-sensei-course-enrolment-provider-interface.php) that handle course enrolment that we could draw inspiration from.

@m1r0
Copy link
Member

m1r0 commented Mar 22, 2022 •
edited

Looks solid to me!

The only thing I can think of is to maybe abstract the filters passed to the data providers (I like the name). For example, the courses currently have a last activity date filter, but the lessons have only a course filter. Right now the data provider interface assumes that all instances will have a last activity date filter, which is not the case. If the filters are abstracted, we could also easily add an enrolment date filter for example.

@Imran92
Copy link
Contributor

Imran92 commented Mar 23, 2022 •
edited

Looks good, a nice design to adopt. I think I'll create a branch based on this one and implement the Reports->Lessons related classes.

@m1r0
Copy link
Member

m1r0 commented Mar 24, 2022

@merkushin Could you merge master? There are a few things missing for the students. 🙂

@merkushin
Copy link
Member Author

@m1r0

@merkushin Could you merge master? There are a few things missing for the students. 🙂

Merged :)

@m1r0
Copy link
Member

m1r0 commented Mar 24, 2022

Merged :)

That was fast!!! ⚡

@m1r0
Copy link
Member

m1r0 commented Mar 25, 2022

@merkushin Sorry, can you do one more merge with master?

@merkushin
Copy link
Member Author

@m1r0

@merkushin Sorry, can you do one more merge with master?

Merged!

@merkushin merkushin added this to the 4.3.0 - Reports v2 milestone Mar 29, 2022
@merkushin merkushin changed the base branch from trunk to feature/reports-refactor March 29, 2022 14:31
@merkushin merkushin changed the title Rough idea of refactoring the structure for Reports Extract get_courses with dependent methods from Sensei_Analysis_Overview_List_Table Mar 29, 2022
@merkushin merkushin requested a review from a team March 29, 2022 14:32
@merkushin merkushin marked this pull request as ready for review March 29, 2022 14:32
m1r0
m1r0 previously approved these changes Mar 29, 2022
View reviewed changes
Copy link
Member

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

Looks good to me. 🙂

@m1r0
Merge pull request #4947 from Automattic/refactor/reports-get-learners
c582358 
Refactor the students overview report code
@merkushin merkushin merged commit cb31337 into feature/reports-refactor Mar 30, 2022
@merkushin merkushin deleted the update/reports-structure-proposal branch March 30, 2022 15:08
@donnapep donnapep mentioned this pull request Apr 5, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Imran92 Imran92 Imran92 left review comments

@m1r0 m1r0 m1r0 approved these changes

Assignees

@merkushin merkushin

Labels
None yet
Projects
None yet
Milestone
4.3.0 - Reports v2
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@merkushin @donnapep @m1r0 @Imran92