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

Update sensei_results_links filter to include the learner's user ID #7048

Merged
merged 9 commits into from
Aug 1, 2023

Conversation

GeoJunkie
Copy link
Contributor

@GeoJunkie GeoJunkie commented Jul 28, 2023

Proposed Changes

Testing Instructions

  1. Install Sensei from this PR and the Sensei Certificates plugin from the corresponding PR.
  2. In WP Admin, navigate to Sensei LMS > Settings > Student Profiles and ensure Public student profiles is checked.
  3. In WP Admin, navigate to Sensei LMS > Settings > Certificate Settings and ensure Public Certificate is checked.
  4. Ensure that there is a user who has completed at least one course that has a certificate.
  5. As that user, navigate to their Learner profile (<site-url>/learner/<login>) and check Allow my Certificates to be publicly viewed. Save the setting.
  6. Navigate to the Completed Courses tab. Ensure the "View Certificate" button is visible.
  7. Open the user's Learner profile in an incognito/private window.
  8. Navigate to the Completed Courses tab. Ensure the "View Certificate" button is visible.
  9. As the original user, uncheck Allow my Certificates to be publicly viewed. Save the setting.
  10. Navigate to the Completed Courses tab. Ensure the "View Certificate" button is visible.
  11. Open the user's Learner profile in an incognito/private window.
  12. Navigate to the Completed Courses tab. Ensure the "View Certificate" button is not visible.

Screenshot of public profile before the update

CleanShot 2023-07-28 at 12 58 18@2x

Screenshot of public profile after the update

CleanShot 2023-07-28 at 12 57 06@2x

New/Updated Hooks

  • Updated application of hook sensei_results_links inside load_user_courses_content() to include a $user_id parameter

New parameter:

  • $user_id: The user ID of the learner being viewed

Pre-Merge Checklist

  • PR title and description contain sufficient detail and accurately describe the changes
  • Acceptance criteria is met
  • Decisions are publicly documented
  • Adheres to coding standards (PHP, JavaScript, CSS, HTML)
  • All strings are translatable (without concatenation, handles plurals)
  • Follows our naming conventions (P6rkRX-4oA-p2)
  • Hooks (p6rkRX-1uS-p2) and functions are documented
  • New UIs are responsive and use a mobile-first approach
  • New UIs match the designs
  • Different user privileges (admin, teacher, subscriber) are tested as appropriate
  • Code is tested on the minimum supported PHP and WordPress versions
  • User interface changes have been tested on the latest versions of Chrome, Firefox and Safari
  • "Needs Documentation" label is added if this change requires updates to documentation
  • Known issues are created as new GitHub issues

@GeoJunkie GeoJunkie changed the title Add sensei_completed_course_links filter Add sensei_completed_course_links filter for use by Sensei Certificates Jul 28, 2023
@GeoJunkie GeoJunkie added the Hooks This change adds or modifies one or more hooks. label Jul 28, 2023
@codecov
Copy link

codecov bot commented Jul 28, 2023

Codecov Report

Merging #7048 (fdbb06b) into trunk (771025e) will increase coverage by 0.04%.
Report is 3 commits behind head on trunk.
The diff coverage is 66.66%.

❗ Current head fdbb06b differs from pull request most recent head b38ae8d. Consider uploading reports for the commit b38ae8d to get more accurate results

Impacted file tree graph

@@             Coverage Diff              @@
##              trunk    #7048      +/-   ##
============================================
+ Coverage     48.03%   48.07%   +0.04%     
- Complexity    10465    10467       +2     
============================================
  Files           573      573              
  Lines         44151    44164      +13     
  Branches        402      402              
============================================
+ Hits          21207    21234      +27     
+ Misses        22617    22603      -14     
  Partials        327      327              
Files Changed Coverage Δ
includes/class-sensei-course.php 38.51% <0.00%> (+0.02%) ⬆️
includes/internal/installer/class-schema.php 100.00% <100.00%> (ø)

... and 3 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8da6d0a...b38ae8d. Read the comment docs.

@GeoJunkie GeoJunkie changed the title Add sensei_completed_course_links filter for use by Sensei Certificates Add sensei_results_completed_links filter for use by Sensei Certificates Jul 28, 2023
@GeoJunkie GeoJunkie added this to the 4.16.1 milestone Jul 28, 2023
@GeoJunkie GeoJunkie self-assigned this Jul 28, 2023
@GeoJunkie GeoJunkie marked this pull request as ready for review July 28, 2023 18:40
@GeoJunkie GeoJunkie requested a review from a team July 28, 2023 20:10
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.

Hey Mike, thanks for the help, and glad to see you again!

I've left some feedback, so let me know what you think.

*
* @return {string} HTML output of the links.
*/
$complete_html .= apply_filters( 'sensei_results_completed_links', '', $course_item->ID, $user->ID );
Copy link
Member

Choose a reason for hiding this comment

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

Instead of introducing a new filter, do you think we can reuse the old one? I was thinking that we might refactor a bit to make the hook a bit more accessible. Let me know what you think of the following patch:

diff --git a/includes/class-sensei-course.php b/includes/class-sensei-course.php
index 5df6d9d9a..67d4d6388 100755
--- a/includes/class-sensei-course.php
+++ b/includes/class-sensei-course.php
@@ -2067,41 +2067,23 @@ class Sensei_Course {

                                                $complete_html .= $this->get_progress_meter( 100 );

-                               if ( $manage ) {
-                                       $has_quizzes = Sensei()->course->course_quizzes( $course_item->ID, true );
-
-                                       // Output only if there is content to display
-                                       if ( self::has_results_links( $course_item->ID ) || $has_quizzes ) {
-                                               $complete_html .= '<p class="sensei-results-links">';
-                                               $results_link   = '';
-
-                                               if ( $has_quizzes ) {
-                                                       $results_link = '<a class="button view-results" href="'
-                                                               . esc_url( self::get_view_results_link( $course_item->ID ) )
-                                                               . '">' . esc_html__( 'View Results', 'sensei-lms' )
-                                                               . '</a>';
-                                               }
-
-                                               /**
-                                                * Filter documented in Sensei_Course::the_course_action_buttons
-                                                */
-                                               $complete_html .= apply_filters( 'sensei_results_links', $results_link, $course_item->ID );
-                                               $complete_html .= '</p>';
+                               $results_link = '';
+                               if ( $manage && Sensei()->course->course_quizzes( $course_item->ID, true ) ) {
+                                       $results_link = '<a class="button view-results" href="'
+                                               . esc_url( self::get_view_results_link( $course_item->ID ) )
+                                               . '">' . esc_html__( 'View Results', 'sensei-lms' )
+                                               . '</a>';
+                               }

-                                       }
+                               /**
+                                * Filter documented in Sensei_Course::the_course_action_buttons
+                                */
+                               $results_links = apply_filters( 'sensei_results_links', $results_link, $course_item->ID );
+                               if ( $results_links ) {
+                                       $complete_html .= '<p class="sensei-results-links">';
+                                       $complete_html .= $results_links;
+                                       $complete_html .= '</p>';
                                }
-                                       /**
-                                        * Publicly displays links related to the completed course
-                                        *
-                                        * @since 4.16.1
-                                        * @hook sensei_results_completed_links
-                                        *
-                                        * @param {int} $course_id The ID of the course.
-                                        * @param {int} $user_id The user ID of the learner.
-                                        *
-                                        * @return {string} HTML output of the links.
-                                        */
-                                       $complete_html .= apply_filters( 'sensei_results_completed_links', '', $course_item->ID, $user->ID );

                                        $complete_html .= '</section>';

This should preserve the old logic and make it easier for third parties to hook in. I don't think we need the self::has_results_links check in this case, so I've removed it from the logic.

LMKWYT.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @m1r0 !

I'm all for this if it won't have any unintended side effects. I added a new filter thinking having it applied inside the $manage was intentional but if it isn't, this makes sense.

If that filter isn't supposed to be suppressed like that, your patch makes perfect sense!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@m1r0 I had to make one update to apply the leaner's user ID to the apply_filter() statement here. That enables us to determine if a user has access to view the certificate. I made the change in 03a4382

changelog/add-profile-links-filter Outdated Show resolved Hide resolved
@GeoJunkie GeoJunkie changed the title Add sensei_results_completed_links filter for use by Sensei Certificates Update sensei_results_links filter to include the learner's user ID Aug 1, 2023
@GeoJunkie GeoJunkie requested a review from m1r0 August 1, 2023 17:59
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, thanks for the contribution! 🎉

@m1r0 m1r0 merged commit 50a2b34 into Automattic:trunk Aug 1, 2023
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Hooks This change adds or modifies one or more hooks.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants