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

Add AutomateWoo course actions #6849

Closed
wants to merge 12 commits into from
Closed

Add AutomateWoo course actions #6849

wants to merge 12 commits into from

Conversation

m1r0
Copy link
Member

@m1r0 m1r0 commented May 1, 2023

This PR adds 2 new AutomateWoo course actions.

Proposed Changes

  • A new action for adding a student to a course.
  • A new action for removing a student from a course.

Testing Instructions

  1. Run composer install to update the autoloader.
  2. Install the AutomateWoo plugin.
  3. Go to AutomateWoo -> Workflows.
  4. Create a new workflow. For the trigger select "Student Added To the Group" (you may choose another if you don't have Sensei Pro). For the action use "Add to Course" and select a few courses.
  5. Create a new workflow. For the trigger select "Student Removed From the Group" (you may choose another if you don't have Sensei Pro). For the action use "Remove from Course" and select the same courses.
  6. Go to Sensei LMS -> Groups and add a user to a group.
  7. Make sure the user is also added to the selected courses by the "Add to Course" action.
  8. Now remove the same user from a group and make sure the user is removed from the selected courses by the "Remove from Course" action.

I was not able to add tests for the AutomateWoo actions because they are extending an abstraction class provided by the AutomateWoo plugin.

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

@m1r0 m1r0 self-assigned this May 1, 2023
@codecov
Copy link

codecov bot commented May 1, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (9f25df0) 51.21% compared to head (bd2bd42) 51.20%.
Report is 12 commits behind head on trunk.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##              trunk    #6849      +/-   ##
============================================
- Coverage     51.21%   51.20%   -0.01%     
- Complexity    11188    11189       +1     
============================================
  Files           614      614              
  Lines         47228    47230       +2     
  Branches        405      405              
============================================
  Hits          24186    24186              
- Misses        22715    22717       +2     
  Partials        327      327              

see 2 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 e982589...bd2bd42. Read the comment docs.

@m1r0 m1r0 force-pushed the add/automatewoo-actions branch from a9d9f2b to 2c630c1 Compare May 1, 2023 17:28
@m1r0 m1r0 marked this pull request as ready for review May 1, 2023 17:39
@m1r0 m1r0 requested a review from a team May 1, 2023 17:43
Copy link
Contributor

@gabrielcaires gabrielcaires left a comment

Choose a reason for hiding this comment

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

Screenshot 2023-06-09 at 16 23 41 I am not having success to find the new actions. Am I doing something wrong? 🤔

@m1r0

@gabrielcaires gabrielcaires added this to the 4.16.0 milestone Jun 14, 2023
@m1r0
Copy link
Member Author

m1r0 commented Jun 14, 2023

I am not having success to find the new actions. Am I doing something wrong?

@gabrielcaires Oh, you may have to run composer install to update the autoloader because of the new files.

Copy link
Contributor

@gabrielcaires gabrielcaires left a comment

Choose a reason for hiding this comment

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

I have had success running after the composer install!

I found the following points.

  1. I saw the fields Groups and Courses are not required, but I tried to use them without fill the values, and I couldn't see results. Should we mark the fields are required?

image

I also tried to have the Groups field empty and select a course. I was expecting the action to be triggered when I remove a student from any group, but I couldn't see actions.

  1. I succeeded in seeing the Add to Course action working ( setting the group and the course), but I couldn't see the Remove from Course action working, I set a group and a course but the student was not removed.

@merkushin
Copy link
Member

Hey @m1r0 👋 Just pinging you here as it looks like this PR awaits your reaction :)

@donnapep
Copy link
Collaborator

I couldn't get this to work, even after running composer install again. I basically wanted to check if the trigger is actually named "Student Removed To the Group". If so, it should be "Student Removed from the Group". 🙂

@donnapep donnapep removed this from the 4.16.0 milestone Jul 11, 2023
@donnapep donnapep requested a review from a team January 25, 2024 14:21
Copy link
Collaborator

@donnapep donnapep left a comment

Choose a reason for hiding this comment

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

@m1r0 Looks like Gabriel's comment and mine are still outstanding.

@m1r0
Copy link
Member Author

m1r0 commented Jan 26, 2024

Thanks for the reminder, Donna. This one slipped under the radar. I'm adding it to my todo. ✔️

@m1r0 m1r0 added this to the 4.20.2 milestone Feb 5, 2024
@m1r0
Copy link
Member Author

m1r0 commented Feb 5, 2024

I saw the fields Groups and Courses are not required, but I tried to use them without fill the values, and I couldn't see results. Should we mark the fields are required?

Makes sense for the Course field. I've updated the code 31604ff.

As for the groups, I think this is out of scope. This PR only adds the course actions. Also, note that the groups logic actually lives in the AutomateWoo plugin.

I succeeded in seeing the Add to Course action working ( setting the group and the course), but I couldn't see the Remove from Course action working, I set a group and a course but the student was not removed.

Hm, it seems to be working for me. Could you double-check/record a video?

2bawtj.mov

I couldn't get this to work, even after running composer install again.

Donna, are the actions not triggering or you can't see the course actions at all? Note that the actions are at the bottom of the list.

cFsiX6.mp4

I basically wanted to check if the trigger is actually named "Student Removed To the Group". If so, it should be "Student Removed from the Group".

The name is "Student Removed from the Group", it was wrong just in the testing instructions. 🙂 FYI, that code is part of the AutomateWoo plugin.

@m1r0 m1r0 requested a review from a team February 5, 2024 19:05
@m1r0 m1r0 modified the milestones: 4.20.2, 4.20.3 Feb 7, 2024
Copy link
Collaborator

@donnapep donnapep left a comment

Choose a reason for hiding this comment

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

It's working for me now. Apparently the secret is to wait 6 months and try again! 😅

After testing it out, I'm not clear on why we feel this change is necessary, since it looks like this functionality is already part of Groups. What I mean is that you can already add courses to a group, and when a student is added to the group it also adds them to those courses. And same thing when you remove a student from a group:

Screenshot 2024-02-14 at 11 58 00 AM

Unless I'm missing some context, this seems like 2 different ways of doing the same thing. Is there some additional value that this integration offers that Groups doesn't?

In case the value is outside of the Group trigger, I think we may need to map this out as it doesn't make sense, for example, to have the Course Completed trigger add or remove a student to / from a course. That just seems like a recipe for disaster. 😳 Which leads me back to wanting to understand the overall goal of this change. 🙂

In case I am missing some important context, I wanted to add some other thoughts before I forget them.

Makes sense for the Course field. I've updated the code 31604ff.

This doesn't seem to work as I can save without adding a course.

As for the groups, I think this is out of scope. This PR only adds the course actions. Also, note that the groups logic actually lives in the AutomateWoo plugin.

It does? So this is something that AutomateWoo added to support Sensei? If so, although it may be considered out of scope for this PR, I do think we need to address it as it's misleading. At a minimum we could open an issue in the AutomateWoo repo; at best we would fix it too, although I think it would enable maximum flexibility to make it work with the Groups field empty.

@m1r0
Copy link
Member Author

m1r0 commented Feb 15, 2024

Unless I'm missing some context, this seems like 2 different ways of doing the same thing. Is there some additional value that this integration offers that Groups doesn't?

No problem, adding the course actions was an idea that Ronnie brought up, so I can see why there's a knowledge gap.

The main idea was to give users more flexibility and help with automation in general. For example, with AutomateWoo, a site owner can gift a course when a user's total orders reach a threshold, or enroll a user in multiple courses when a product is purchased.

The testing instructions use the group triggers just because it's a known territory. The feature is not strictly related to that, you can use any other trigger.

This doesn't seem to work as I can save without adding a course.

This is an AutomateWoo thing. It allows saving even if you don't fill the required fields. You can observe the same when omitting to select the trigger. The workflow will do nothing if no course or trigger is selected.

If so, although it may be considered out of scope for this PR, I do think we need to address it as it's misleading.

Could you be more specific about this? I'm not sure what the end goal is. Is it to make selecting a group a required action?

@donnapep
Copy link
Collaborator

The main idea was to give users more flexibility and help with automation in general. For example, with AutomateWoo, a site owner can gift a course when a user's total orders reach a threshold, or enroll a user in multiple courses when a product is purchased.

These use cases make sense, though I'm concerned about introducing potential issues if the user selects an undesirable combination (e.g. Signed Up to a Course + Remove from Course). Is it possible to filter the actions based on which trigger is selected? If so, I think we should map which triggers make sense for the "remove from / add to course" actions and which don't. If not, do you think the benefits outweigh the risks of users selecting problematic combinations of trigger & action?

@merkushin merkushin modified the milestones: 4.21.0, 4.21.1 Feb 27, 2024
@m1r0
Copy link
Member Author

m1r0 commented Mar 12, 2024

I'm concerned about introducing potential issues if the user selects an undesirable combination

Makes sense, it would be best to avoid that if we can.

Is it possible to filter the actions based on which trigger is selected?

I'm not sure to be honest. The only thing that I can see that can limit the action is the $required_data_items which is not useful in our case. I don't know the codebase that much so there could be something else but the docs are not very helpful.

Taking the above into consideration, and knowing that AutomateWoo already has some Sensei triggers, I think it would be best to integrate this into AutomateWoo directly. That way we would have the expertise of the AutomateWoo team and be able to add proper tests. I'm closing this PR.

@m1r0 m1r0 closed this Mar 12, 2024
@donnapep donnapep deleted the add/automatewoo-actions branch March 13, 2024 19:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants