Automattic / sensei

Sensei LMS - Online Courses, Quizzes, & Learning
https://senseilms.com
GNU General Public License v2.0
545 stars 198 forks source link

Handle empty module on the course structure saving #7649

Closed merkushin closed 3 months ago

merkushin commented 3 months ago

Resolves #7625

The problem described in the issue happens when, while following the tour, the user doesn't set the module title.

The course structure can't be saved in this case, and it causes the issue that the expected “Edit Lesson” link doesn't appear in the toolbar.

Proposed Changes

Testing Instructions

  1. On a new website, create a course, and follow the next steps before saving/publishing the course.
  2. Take the tour and on the third step (“Adding a module”) make sure we mention the need to add the title for the module: “Then click the Module option and give it a name.”
  3. Add a few modules without setting titles for them.
  4. Save the draft.
  5. Make sure each module without a name got a title like “Module N” where N is an ordinal number.

Pre-Merge Checklist

github-actions[bot] commented 3 months ago

Test the previous changes of this PR with WordPress Playground.

github-actions[bot] commented 3 months ago

WordPress Dependencies Report

The github-action-wordpress-dependencies-report action has detected some script changes between the commit 8eab7587ca9c02bd7ee960b4f97ce54a1b957e95 and trunk. Please review and confirm the following are correct before merging.

Script Handle Added Dependencies Removed Dependencies Total Size Size Diff
admin/tour/course-tour/index.js 35.9 kB +8 B ( +0.03% 🔼 )

This comment was automatically generated by the github-action-wordpress-dependencies-report action.

github-actions[bot] commented 3 months ago

Test the previous changes of this PR with WordPress Playground.

renatho commented 3 months ago

Hey! 👋 Just a suggestion. I thought we would implement it as a solution applied only to the tour (populating the module names if needed as part of one of the steps). Wouldn't it be better?

Imran92 commented 3 months ago

Thanks for raising it @renatho !

Hey! 👋 Just a suggestion. I thought we would implement it as a solution applied only to the tour (populating the module names if needed as part of one of the steps). Wouldn't it be better?

It was exactly my thought and I agree it would be better (That's why my suggestion here p1722358285119989/1722355438.888029-slack-C02NWDZBL0H was to add the names before "triggering" save, we trigger a save during the tour). I thought about commenting the same here, but later thought as this is already implemented, it'd make us re-do the whole thing, that's why I approved it, thinking about the cost vs. benefit. But I would've preferred doing it only for the tour to keep the flow of the tour smooth and errorless, and enforce user to explicitly give modules a Title as before and not saving otherwise in rest of the cases.

merkushin commented 3 months ago

@renatho @Imran92

I thought we would implement it as a solution applied only to the tour (populating the module names if needed as part of one of the steps). Wouldn't it be better?

That's why my suggestion here p1722358285119989/1722355438.888029-slack-C02NWDZBL0H was to add the names before "triggering" save, we trigger a save during the tour.

Yes, initially tried to do that on the frontend, but didn't find a proper way to update titles, and switched to a more straightforward solution.

Also, speaking about the frontend, I noticed that we only show the error message at the top of the screen, but don't even highlight the fields that require user's action (and I think because of placeholders it might not be obvious for some users). So I was thinking that setting the default title on the backend is a quick solution to this problem as well as it forgives some user mistakes, and we don't need to work on UX and design for that.

I'm going to look maybe for a quick dirty solution for the approach Imran suggested.

merkushin commented 3 months ago

@renatho @Imran92 here is the new PR with setting titles only in the tour: https://github.com/Automattic/sensei/pull/7652