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

Handle empty module on the course structure saving #7649

Closed
wants to merge 4 commits into from
Closed

Handle empty module on the course structure saving #7649

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
464f517
Set the default name for modules with empty titles
merkushin Jul 31, 2024
183171a
Test setting default names for modules without titles
merkushin Jul 31, 2024
4738c7a
Add changelog entry
merkushin Jul 31, 2024
8eab758
Don't enforce the return type for the closure
merkushin Jul 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion assets/admin/tour/course-tour/steps.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,7 @@ function getTourSteps() {
heading: __( 'Adding a module', 'sensei-lms' ),
descriptions: {
desktop: __(
'A module is a container for a group of related lessons in a course. Click + to open the inserter. Then click the Module option.',
'A module is a container for a group of related lessons in a course. Click + to open the inserter. Then click the Module option and give it a name.',
'sensei-lms'
),
mobile: null,
Expand Down
4 changes: 4 additions & 0 deletions changelog/fix-handle-empty-module-on-save
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Significance: patch
Type: fixed

Set default names for modules without titles
26 changes: 20 additions & 6 deletions config/psalm/psalm-baseline.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1297,6 +1297,9 @@
<InvalidScalarArgument occurrences="1">
<code>$course_name</code>
</InvalidScalarArgument>
<MissingClosureReturnType occurrences="1">
<code>function ( $item ) use ( &amp;$existing_module_names, &amp;$i ) {</code>
</MissingClosureReturnType>
<NullableReturnStatement occurrences="1">
<code>$term-&gt;term_id</code>
</NullableReturnStatement>
Expand Down Expand Up @@ -3558,6 +3561,9 @@
<code>null === $first</code>
<code>null === $second</code>
</DocblockTypeContradiction>
<InvalidAttribute occurrences="1">
<code>\ReturnTypeWillChange</code>
</InvalidAttribute>
<InvalidReturnStatement occurrences="1">
<code>wp_delete_attachment( $file_id, true )</code>
</InvalidReturnStatement>
Expand Down Expand Up @@ -3616,6 +3622,9 @@
<code>! self::$instance</code>
<code>self::$instance</code>
</DocblockTypeContradiction>
<InvalidAttribute occurrences="1">
<code>\ReturnTypeWillChange</code>
</InvalidAttribute>
<InvalidReturnStatement occurrences="2">
<code>$this-&gt;create_job( $user_id, Sensei_Export_Job::class )</code>
<code>$this-&gt;create_job( $user_id, Sensei_Import_Job::class )</code>
Expand Down Expand Up @@ -4286,6 +4295,9 @@
</RedundantPropertyInitializationCheck>
</file>
<file src="includes/enrolment/class-sensei-course-enrolment-provider-results.php">
<InvalidAttribute occurrences="1">
<code>\ReturnTypeWillChange</code>
</InvalidAttribute>
<InvalidScalarArgument occurrences="1">
<code>$version</code>
</InvalidScalarArgument>
Expand Down Expand Up @@ -4394,10 +4406,18 @@
<InvalidArgument occurrences="1">
<code>$provider_states</code>
</InvalidArgument>
<InvalidAttribute occurrences="1">
<code>\ReturnTypeWillChange</code>
</InvalidAttribute>
<InvalidPropertyAssignmentValue occurrences="1">
<code>$provider_states</code>
</InvalidPropertyAssignmentValue>
</file>
<file src="includes/enrolment/class-sensei-enrolment-provider-state.php">
<InvalidAttribute occurrences="1">
<code>\ReturnTypeWillChange</code>
</InvalidAttribute>
</file>
<file src="includes/hooks/template.php">
<ArgumentTypeCoercion occurrences="1">
<code>'Sensei_Main'</code>
Expand Down Expand Up @@ -5255,12 +5275,6 @@
</UndefinedPropertyFetch>
</file>
<file src="includes/rest-api/class-sensei-rest-api-question-helpers-trait.php">
<ArgumentTypeCoercion occurrences="1">
<code>$post_args</code>
</ArgumentTypeCoercion>
<InvalidArgument occurrences="1">
<code>$post_args</code>
</InvalidArgument>
<PossibleRawObjectIteration occurrences="1">
<code>$question_categories</code>
</PossibleRawObjectIteration>
Expand Down
51 changes: 50 additions & 1 deletion includes/class-sensei-course-structure.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,7 +230,8 @@ private function get_modules(): array {
* @return bool|WP_Error
*/
public function save( array $raw_structure ) {
$structure = $this->sanitize_structure( $raw_structure );
$raw_structure = $this->ensure_modules_have_names( $raw_structure );
Copy link
Contributor

Choose a reason for hiding this comment

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

Has this check become redundant now?

sensei/includes/class-sensei-course-structure.php

Lines 897 to 902 in 53dbffa

if ( 'module' === $raw_item['type'] ) {
return new WP_Error(
'sensei_course_structure_modules_missing_title',
__( 'Please ensure all modules have a name before saving.', 'sensei-lms' )
);
}

$structure = $this->sanitize_structure( $raw_structure );
if ( is_wp_error( $structure ) ) {
return $structure;
}
Expand Down Expand Up @@ -713,6 +714,54 @@ private function flatten_structure( array $structure ): array {
];
}

/**
* Check if modules have titles and set the default ones otherwise.
*
* @param array $raw_structure Structure array as returned by get().
*
* @return array Structure array with module titles set.
*/
private function ensure_modules_have_names( array $raw_structure ) {
$existing_module_names = array_map(
function ( $item ) {
if ( ! isset( $item['type'], $item['title'] ) ) {
return null;
}
return 'module' === $item['type'] ? $item['title'] : null;
},
$raw_structure
);
$existing_module_names = array_filter( $existing_module_names );

$i = 0;
$raw_structure = array_map(
function ( $item ) use ( &$existing_module_names, &$i ) {
// Ignore items with improper structure.
if ( ! isset( $item['type'], $item['title'] ) ) {
return $item;
}

// Ignore items that are not modules or have a title already.
if ( 'module' !== $item['type'] || ! empty( $item['title'] ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: Just an edge case as we are trying to work against user-made scenarios - can we trim the title and then check if it's empty? Because otherwise, it doesn't assign a name to the module if the title is just one or more spaces. Does it make sense?

return $item;
}

do {
// translators: Placeholder value is an ordinal number.
$module_title = sprintf( __( 'Module %d', 'sensei-lms' ), ++$i );
} while ( in_array( $module_title, $existing_module_names, true ) );

$item['title'] = $module_title;
$existing_module_names[] = $module_title;

return $item;
},
$raw_structure
);

return $raw_structure;
}

/**
* Parse, validate, and sanitize the structure input.
*
Expand Down
56 changes: 52 additions & 4 deletions tests/unit-tests/test-class-sensei-course-structure.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1732,10 +1732,10 @@ public function testSave_WithNewLesson_FiresLessonCreatedAction() {
$course_structure = Sensei_Course_Structure::instance( $course_id );

$lesson_created_action_fired = false;
$action = function( $lesson_id, $course_id ) use ( &$lesson_created_action_fired ) {
$action = function () use ( &$lesson_created_action_fired ) {
$lesson_created_action_fired = true;
};
add_action( 'sensei_course_structure_lesson_created', $action, 10, 2 );
add_action( 'sensei_course_structure_lesson_created', $action, 10 );

/* Act. */
$course_structure->save( $new_structure );
Expand All @@ -1760,15 +1760,63 @@ public function testSave_WithNewLesson_FiresQuizCreatedAction() {
$course_structure = Sensei_Course_Structure::instance( $course_id );

$quiz_created_action_fired = false;
$action = function( $quiz, $lesson ) use ( &$quiz_created_action_fired ) {
$action = function () use ( &$quiz_created_action_fired ) {
$quiz_created_action_fired = true;
};
add_action( 'sensei_quiz_create', $action, 10, 2 );
add_action( 'sensei_quiz_create', $action, 10 );

/* Act. */
$course_structure->save( $new_structure );

/* Assert. */
$this->assertTrue( $quiz_created_action_fired );
}


/**
* Tests to ensure items without types fail save.
*/
public function testSave_ModulesWithoutTitles_SavesWithDefaultTitles() {
/* Arrange. */
$this->login_as_teacher();

$course_id = $this->factory->course->create();

$new_structure = array(
array(
'type' => 'module',
'title' => '',
'lessons' => array(),
),
array(
'type' => 'module',
'title' => 'Module 2',
'lessons' => array(),
),
array(
'type' => 'module',
'title' => '',
'lessons' => array(),
),
);

$course_structure = Sensei_Course_Structure::instance( $course_id );

/* Act. */
$course_structure->save( $new_structure );

/* Assert. */
$structure = $course_structure->get( 'edit' );
$actual = $this->extractTitlesFromItems( $structure );
$expected = array( 'Module 1', 'Module 2', 'Module 3' );
$this->assertSame( $expected, $actual );
}

private function extractTitlesFromItems( $items ) {
$titles = array();
foreach ( $items as $item ) {
$titles[] = $item['title'];
}
return $titles;
}
}
Loading