-
Notifications
You must be signed in to change notification settings - Fork 9
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
Rake task and service for publishing a course #4698
Conversation
fb3bbdf
to
c0c0aec
Compare
end | ||
course = Course.find_by(uuid:) | ||
|
||
raise UnpublishableError unless course.publishable? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it should raise an error. I want to use this in the controller so I don't want to catch this error.
Should this
- return false if the course is unpulishable?
- return the course and expect the caller to check if the course was published?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd go for a mixed solution 1 and 2.
This is how I'd write this:
module Courses
class CoursePublisher
def initialize(course:, user:)
@course = course
@user = user
end
def call
return false unless @course.publishable?
publish_course
notify_course_published
@course
end
private
def publish_course
Course.transaction do
@course.publish_sites
@course.publish_enrichment(@user)
@course.application_status_open!
end
end
def notify_course_published
NotificationService::CoursePublished.call(course: @course)
end
end
end
And on controller (some fake code to exemplify):
def publish
course = Course.find_by(uuid: params[:uuid])
if result = Courses::CoursePublisher.new(course:, user: current_user).call
redirect_to course_path(result), notice: 'Course published successfully!'
else
redirect_to courses_path, alert: 'Course could not be published.'
end
end
I put the notification out of the transaction. Not sure if it should though.
Also Publish has this pattern of include ServicePattern
. Not sure if I like to suffix everything with Service
though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, I'll incorporate this into the PR
16a9f5b
to
6c523fb
Compare
Extract the process of publishing a course to a service
pass user email and course uuid
6c523fb
to
e5d6dc1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
What do you think about adding a documentation or small content to support playbook?
2b321b7
to
ca771bd
Compare
Context
We sometimes need to publish a course through the console. The main scenario is for when the course is in a previous recruitment cycle.
It's good to have a stable service to run this in a repeatable and safe way
Changes proposed in this pull request
Extract a Service
Courses::PublishService
from the methods in theCourse
model.The service accepts a user and a course
The task accepts a
uuid
and user emailI've used the service in the
Publish::CoursesController#publish
actionGuidance to review