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

Fix lesson order in drip email #172

Merged
merged 2 commits into from Nov 29, 2019
Merged

Fix lesson order in drip email #172

merged 2 commits into from Nov 29, 2019

Conversation

@gkaragia
Copy link
Contributor

gkaragia commented Oct 25, 2019

Fixes #154

Overview

Lessons can be ordered by two separate ways, by module or by lesson.
Previously, the lessons in the emails were only ordered if a lesson order was
defined and the module ordering was ignored. This is now fixed. Course ordering is not considered since we have no pages that display lessons from different courses. However the lessons that belong to the same course will be listed together.

Testing

The automated tests for content drip are not configured to run at the moment. I spent some time trying to add a test and run it but unfortunately it proved to be a time consuming task so I skipped adding automated tests.

Manual test cases that were executed:

  • Ordering is correct when all lessons belong to a module.
  • Ordering is correct when some lessons belong to modules.
  • Ordering is correct when no lessons belong to a module.
  • Lessons do not get mixed up when they belong to different courses.

To test the functionality follow the instructions outlined in #154

Lessons can be ordered by two separate ways, by module or by lesson.
Previously, the lessons in the emails were only ordered if a lesson order was
defined and the module ordering was ignored. This is now fixed.
@gkaragia gkaragia requested review from jom, alexsanford and donnapep Oct 25, 2019
@donnapep

This comment has been minimized.

Copy link
Contributor

donnapep commented Oct 28, 2019

@gkaragia I removed a couple of lessons from modules. They are ordered like so:

Screen Shot 2019-10-28 at 11 48 31 AM

But in the email the lessons are ordered as per the following:

Screen Shot 2019-10-28 at 11 49 26 AM

In other words, "Other Lessons" are not ordered correctly in the email.

@gkaragia

This comment has been minimized.

Copy link
Contributor Author

gkaragia commented Oct 30, 2019

Hey @donnapep I have added extra logic to sort the lessons if they don't belong in a module so this should now work.

There is still a chance for the 'Other lessons' to still not have the same ordering in case that they don't have the order$course_id meta set. The lessons that don't have this meta value are appended in the end with no particular order in both the frontend and the email ordering code.
However, I don't think that there is a high chance to see that without deleting the values from the db manually as when you update the order in wp-admin it sets the 'order$course_id' meta value for all lessons.

@jom
jom approved these changes Nov 8, 2019
Copy link
Contributor

jom left a comment

Looks good and worked well in my tests.

@@ -399,14 +399,27 @@ private function get_ordered_courses_and_lessons( $lessons ) {

// Sort list of lessons for each course.
foreach ( $courses_and_lessons as $course_id => $lesson_data_items ) {
// Set the current order as the default just in case the course lesson order is not set

This comment has been minimized.

Copy link
@jom

jom Nov 8, 2019

Contributor

We've been slowly removing the blank lines padding the inside of control structures. It exists all over the plugin still.

(Also in lines 410+412)

This comment has been minimized.

Copy link
@gkaragia

gkaragia Nov 11, 2019

Author Contributor

I am not sure if we should have rules about blank lines since I think that there can't be a guide which fits in every possible case. In most cases we should do what 'feels' right.

For example for what you suggest it might be ok in some of the cases but it could make the code hard to read in others. For example the code would start to feel cluttered if you had an if/else with 2-3 assignments in each branch and no blank lines or if you had nested if/else statements.

For the specifc case I can see no harm removing the blank line in 402 however removing lines in 410 and 412 I think it makes it a bit harder to read as the if/else together with the second if inside it makes it a bit too compact. What do you think?

This comment has been minimized.

Copy link
@donnapep

donnapep Nov 11, 2019

Contributor

The problem with inconsistency in code styles is that it's quite subjective. While you might leave these blank lines in, someone else would likely come in later and remove them.

I would also argue that multiple if/else and nested if statements are themselves not very readable and can often be refactored to be more succinct and improve clarity.

This comment has been minimized.

Copy link
@gkaragia

gkaragia Nov 11, 2019

Author Contributor

I agree that many things in code styles are subjective. However what I am trying to say is that by introducing such a rule we actually lose clarity, at least for some of the cases, which defeats purpose.

If this rule has been decided already and is not subject to change, I am fine with removing the lines however do we have such decisions documented somewhere so I can be aware of them?

This comment has been minimized.

Copy link
@donnapep

donnapep Nov 15, 2019

Contributor

do we have such decisions documented somewhere so I can be aware of them?

Not to my knowledge.

@gkaragia gkaragia requested a review from jom Nov 11, 2019
@gkaragia gkaragia force-pushed the fix/email-lesson-ordering branch from 170a580 to f7a5620 Nov 27, 2019
@gkaragia

This comment has been minimized.

Copy link
Contributor Author

gkaragia commented Nov 27, 2019

@donnapep @jom should I proceed with merging?

Copy link
Contributor

donnapep left a comment

This works well. 👍 Although we haven't achieved consensus on the whitespace issue, I think it's likely you may find that leaving extra whitespace is contrary to what everyone else is doing. Plus, it makes for longer files. 😄

@donnapep donnapep added this to the 2.0.1 milestone Nov 29, 2019
@gkaragia gkaragia merged commit eeba2e9 into master Nov 29, 2019
@gkaragia gkaragia deleted the fix/email-lesson-ordering branch Nov 29, 2019
@donnapep donnapep changed the title Fix email lesson ordering Fix lesson order in drip email Dec 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.