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

Refactor LLMS_Engagements #1771

Closed
wants to merge 272 commits into from
Closed

Refactor LLMS_Engagements #1771

wants to merge 272 commits into from

Conversation

thomasplevy
Copy link
Contributor

@thomasplevy thomasplevy commented Sep 1, 2021

Description

Refactors a significant portion of certificate and engagement related code, deprecates a lot of code and simplifies the process of generating earned certificates and achievements.

TODO

  • Write achievement tests (there aren't any or possibly any)
  • Replace metadata with post thumbnail for both post types (deprecate the meta properties how certificate title is deprecated)
  • Deprecate achievement title & content like certificate title is deprecated
  • Add extra metadata to achievements (related, engagement, etc...)
  • Is certificate content properly / fully transitioned from metadata to the post content?
  • Rewrite queries on the LLMS_Student model to use a WP_Query. We might have to deprecate the existing methods and create new ones, I haven't looked at this too thoroughly yet. Possibly create custom class for this, like LLMS_Engagements_Query
  • Deprecate the LLMS_Achievments::trigger_engagement() class (like certificates has been deprecated)
  • More tests... I wrote a lot, there's more to write.
  • Do realtime testing as a user
  • Did I break e2e tests completely?
  • Merge this into Allow editing earned certificates and achievements #1838 or merge that into this
  • Gather deprecations and write to changelog/s
  • Write and test database migrations (my_cert and my_achievement) post/meta data
    • postmeta title & content => post title & content
    • Featured image meta => thumbnail
    • store user as post_author
    • store template as post_parent
    • maybe more?
  • Update achievements metabox to remove (deprecated) image (@eri-trabiccolo)

How has this been tested?

  • New tests
  • Existing tests
  • Manually (various tests with delayed & immediate engagments for emails, achievements, and certs)

Screenshots

Types of changes

  • Bug fix
  • Refactor existing code

Checklist:

  • My code has been tested.
  • My code passes all existing automated tests.
  • My code follows the LifterLMS Coding & Documentation Standards.

@thomasplevy thomasplevy changed the base branch from trunk to dev September 1, 2021 23:11
@thomasplevy thomasplevy marked this pull request as ready for review October 15, 2021 21:59
@thomasplevy thomasplevy requested a review from a team October 15, 2021 21:59
Copy link
Collaborator

@eri-trabiccolo eri-trabiccolo left a comment

Choose a reason for hiding this comment

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

I think it's overall good.
Although I think we should avoid sending delayed engagements when the related post has been trashed/deleted, and when the engagement status is not publish.

* @return void
*/
private function __construct() {

if ( defined( 'LLMS_ENGAGEMENT_DEBUG' ) && LLMS_ENGAGEMENT_DEBUG ) {
_deprecated_function( 'Constant: LLMS_ENGAGEMENT_DEBUG', '[version]' );
Copy link
Collaborator

Choose a reason for hiding this comment

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

includes/class.llms.engagements.php Outdated Show resolved Hide resolved
includes/class.llms.engagements.php Outdated Show resolved Hide resolved
includes/class.llms.engagements.php Outdated Show resolved Hide resolved
includes/class.llms.engagements.php Outdated Show resolved Hide resolved
Copy link
Collaborator

@eri-trabiccolo eri-trabiccolo 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, although, and sorry for being nitpick, I think we still miss this part:

what do we do when the related post (course/lesson/access plan etc.) is trashed/deleted?
I'd say that we should unschedule the engagement as well, although I guess we don't have a way to determine what's the scheduled action to unschedule though...

We could, when processing the delayed engagements, at least avoid the certificates/achievements to be earned if the related post doesn't exist anymore. What do you think?

Right?

And I think we should check also that the engagement type is publish, I mean the achievement/email/certificate template actually exist and is publish otherwise what do we "send" (we might end up producing a fatal btw)?

@thomasplevy
Copy link
Contributor Author

@eri-trabiccolo, forgive me for what I have done.

@thomasplevy thomasplevy deleted the engagement-refactoring branch December 13, 2021 23:42
@thomasplevy thomasplevy mentioned this pull request Dec 13, 2021
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants