Skip to content

feat: pagination studio home for courses - #825

Merged
jesperhodge merged 17 commits into
openedx:masterfrom
eduNEXT:jv/feat-home-studio-pagination
Apr 3, 2024
Merged

feat: pagination studio home for courses#825
jesperhodge merged 17 commits into
openedx:masterfrom
eduNEXT:jv/feat-home-studio-pagination

Conversation

@johnvente

@johnvente johnvente commented Feb 6, 2024

Copy link
Copy Markdown
Contributor

Description

This PR adds pagination for the studio home view and makes minor changes to each course card.

NOTE: This needs to be activated by the environment variable ENABLE_HOME_PAGE_COURSE_API_V2 otherwise, it will continue using the old course list

enable this feature flag
new_studio_mfe.use_new_home_page

DEMO

demo_course_home_pagination.mp4
Screen Shot 2024-03-22 at 1 08 51 PM

Course cards

BEFORE NOW

Supporting information

To review a little bit more about this implementation, please check this:
https://openedx.atlassian.net/wiki/spaces/OEPM/pages/4032856101/Studio+Home+incremental+upgrades+-+product+approach

How to test it

  1. Set up your edx-platform

     cd path-to-your-edx-platform
     git fetch origin pull/34175/head:MJG/course-home-api-v2-pagination
     git checkout MJG/course-home-api-v2-pagination
    
  2. Check it out this branch inside frontend-app-course-authoring

     cd path-to-frontend-app-course-authoring
     git fetch origin pull/825/head:jv/feat-home-studio-pagination
     git checkout jv/feat-home-studio-pagination
     npm start
    
  3. The default items per page is 10 so you would need to create more than 10 courses

You can create this tutor plugin to enable it

name: mfe_course_authoring_config
version: 0.1.0
patches:
  mfe-lms-development-settings: |
    MFE_CONFIG["ENABLE_HOME_PAGE_COURSE_API_V2"]=True

  mfe-lms-production-settings: |
    MFE_CONFIG["ENABLE_HOME_PAGE_COURSE_API_V2"]=True

@openedx-webhooks

openedx-webhooks commented Feb 6, 2024

Copy link
Copy Markdown

Thanks for the pull request, @johnvente! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Feb 6, 2024
@johnvente
johnvente force-pushed the jv/feat-home-studio-pagination branch from bf36663 to 69fec99 Compare February 6, 2024 22:00
@codecov

codecov Bot commented Feb 6, 2024

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 92.72727% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 91.93%. Comparing base (784a811) to head (1bdf719).
Report is 8 commits behind head on master.

Files Patch % Lines
src/studio-home/hooks.jsx 75.00% 2 Missing ⚠️
src/studio-home/tabs-section/courses-tab/index.jsx 85.71% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #825      +/-   ##
==========================================
+ Coverage   91.87%   91.93%   +0.06%     
==========================================
  Files         574      571       -3     
  Lines       10078    10230     +152     
  Branches     2157     2228      +71     
==========================================
+ Hits         9259     9405     +146     
- Misses        791      798       +7     
+ Partials       28       27       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@johnvente
johnvente marked this pull request as ready for review February 6, 2024 22:09
@johnvente
johnvente force-pushed the jv/feat-home-studio-pagination branch from 69fec99 to 75832f8 Compare February 7, 2024 00:58
@johnvente
johnvente force-pushed the jv/feat-home-studio-pagination branch from 75832f8 to 94be906 Compare February 7, 2024 01:12
@mphilbrick211
mphilbrick211 requested a review from a team February 7, 2024 15:09
Comment thread src/studio-home/data/api.js Outdated
Comment thread src/studio-home/factories/mockApiResponses.jsx Outdated
Comment thread src/studio-home/card-item/index.jsx Outdated
Comment thread src/studio-home/factories/mockApiResponses.jsx Outdated
Comment thread src/studio-home/data/slice.js Outdated
Comment thread src/studio-home/data/slice.js Outdated
Comment thread src/studio-home/data/thunks.js Outdated
@johnvente
johnvente force-pushed the jv/feat-home-studio-pagination branch from 95835b5 to d6e2e91 Compare February 8, 2024 18:15

@mariajgrimaldi mariajgrimaldi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

All my comments were addressed, so this is looking good to me!

@johnvente
johnvente force-pushed the jv/feat-home-studio-pagination branch from 3d3d359 to 1f6d30f Compare February 8, 2024 19:44
Comment thread src/studio-home/card-item/index.jsx Outdated
@mphilbrick211

Copy link
Copy Markdown

@openedx/2u-tnl this is ready for review!

@mphilbrick211 mphilbrick211 added the waiting for eng review PR is ready for review. Review and merge it, or suggest changes. label Feb 21, 2024
@johnvente
johnvente requested a review from a team as a code owner March 19, 2024 15:34
@mariajgrimaldi

Copy link
Copy Markdown
Member

I was testing this out in one of my environments and found that when a user doesn't have course creator permissions, this is what is shown:
image
This also happens when no courses are listed for the current user. I'm guessing that's the number of pages. Should we remove it?

@johnvente

johnvente commented Mar 19, 2024

Copy link
Copy Markdown
Contributor Author

I was testing this out in one of my environments and found that when a user doesn't have course creator permissions, this is what is shown:This also happens when no courses are listed for the current user. I'm guessing that's the number of pages. Should we remove it?

Thanks for testing this, it was the number of courses that was being shown I added a fix for that

@johnvente

Copy link
Copy Markdown
Contributor Author

Hi @arbrandes! Could take a look over here please? Thanks a lot

@mariajgrimaldi

Copy link
Copy Markdown
Member

In the backend, we're using a feature toggle to turn on/off the API: FEATURES['ENABLE_HOME_PAGE_COURSE_API_V2']. Can we use something similar here and in #846?

@johnvente

Copy link
Copy Markdown
Contributor Author

In the backend, we're using a feature toggle to turn on/off the API: FEATURES['ENABLE_HOME_PAGE_COURSE_API_V2']. Can we use something similar here and in #846?

That said I added the feature with the same name as it is in backend ENABLE_HOME_PAGE_COURSE_API_V2

@jesperhodge

Copy link
Copy Markdown
Contributor

Hi @johnvente , I'm reviewing this for TNL. In addition, I'm also looking at loading times of studio home -> libraries tab. I wanted to ask if you or edunext are planning to implement pagination for the libraries page as well? I want to avoid conflicts or duplications there.

@johnvente

Copy link
Copy Markdown
Contributor Author

Hi @jesperhodge we aren't planning to implement pagination for the libraries tab, our scope is only for courses tab

@mariajgrimaldi
mariajgrimaldi self-requested a review March 22, 2024 15:24
Comment thread src/studio-home/card-item/CardItem.test.jsx
Comment thread src/studio-home/tabs-section/TabsSection.test.jsx
@mariajgrimaldi

mariajgrimaldi commented Mar 22, 2024

Copy link
Copy Markdown
Member

The first time the pagination loads, it shows: Showing 44 of 44. Here's a video:

Screencast.from.22-03-24.12.40.12.mp4

@mariajgrimaldi mariajgrimaldi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I tested this in my local environment, and the pagination is working as expected. I turned it on/off, and tested that everything worked as before. Thank you for addressing all my comments!

@johnvente

johnvente commented Mar 22, 2024

Copy link
Copy Markdown
Contributor Author

I won't be able to review this in a week, so I will check for any changes on April 1st

@mariajgrimaldi

mariajgrimaldi commented Apr 1, 2024

Copy link
Copy Markdown
Member

Hello there, @jesperhodge @arbrandes! Can you help us with a review? Let us know!

@jesperhodge jesperhodge left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks very good to me! I just left some very minor comments but everything else looks great.
Apologies for not responding here for a week, I was out sick.

Comment thread .env.test Outdated
Comment thread src/studio-home/StudioHome.jsx Outdated
Comment thread src/studio-home/data/api.test.js Outdated
Comment thread src/studio-home/hooks.jsx Outdated
@johnvente
johnvente force-pushed the jv/feat-home-studio-pagination branch from dae083d to 1bdf719 Compare April 2, 2024 20:23

@jesperhodge jesperhodge left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks a lot, John! Again, sorry for the late review. This is great work, super happy to have pagination now!!

@jesperhodge
jesperhodge merged commit fde3872 into openedx:master Apr 3, 2024
@openedx-webhooks

Copy link
Copy Markdown

@johnvente 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

open-source-contribution PR author is not from Axim or 2U

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

9 participants