Fix self paced course content visiblity #11452
Conversation
mushtaqak
changed the title
mushtaqak
force-pushed
the
mushtaq/ECOM-3615-fix-self-paced-scheduled-content
branch
from
1f97e09
to
e474196
Compare
…in Studio ECOM-3615
mushtaqak
force-pushed
the
mushtaq/ECOM-3615-fix-self-paced-scheduled-content
branch
from
e474196
to
f9c3759
Compare
|
@adampalay @awaisdar001 May you please review |
| if course: | ||
| try: | ||
| return course.self_paced and SelfPacedConfiguration.current().enabled | ||
| except AttributeError: |
There was a problem hiding this comment.
do we need to handle the case where no SelfPacedConfiguration is set?
There was a problem hiding this comment.
Not Actually. SelfPacedConfiguration inherits ConfigurationModel which has current() method that creates a dict and returns it. So if it is not set, it returns enabled=False in the dict when we call SelfPacedConfiguration.current()
| """ | ||
| Returns True if course is self-paced, False otherwise. | ||
| """ | ||
| return course.self_paced and SelfPacedConfiguration.current().enabled if course else False |
There was a problem hiding this comment.
nit: I think this is cleaner:
return course and course.self_paced and SelfPacedConfiguration.current().enabled
BTW, what happens if there is no entry in SelfPacedConfiguration?
There was a problem hiding this comment.
If there is no entry in SelfPacedConfiguration it would return false.
… method improvement
|
|
||
| with self.store.default_store(store_type): | ||
| # Create course, chapter and setup future release date to make chapter in scheduled state | ||
| course = CourseFactory.create() |
There was a problem hiding this comment.
it's cleaner to pass store_type to the default_store kwarg in the create method
|
some nits, o/w 👍 |
|
|
||
| # Check that has scheduled state | ||
| xblock_info = self._get_xblock_info(chapter.location) | ||
| self._verify_visibility_state(xblock_info, VisibilityState.ready) |
There was a problem hiding this comment.
@mushtaqak xblock_info will also be in live state? Is it worth checking that?
There was a problem hiding this comment.
@awaisdar001 xblock_info will only have one state at a time. So it would either have ready state or live visibility state. So it would be redundant to check for live state if we already have checked for ready state.
| is_live = datetime.now(UTC) > xblock.start | ||
| children = child_info and child_info.get('children', []) | ||
| if children and len(children) > 0: | ||
| is_live = is_course_self_paced or datetime.now(UTC) > xblock.start |
There was a problem hiding this comment.
I have a suggestion, Instead of passing is_course_self_paced in the method argument would require the param to pass everywhere the method will be used.
How about something like this?
is_live = is_self_paced(course) or datetime.now(UTC) > xblock.start
There was a problem hiding this comment.
Nice catch, but I didn't wanted to pass whole course object which was unnecessary, so I passed only what is required in _compute_visibility_state method.
However, I should make is_course_self_paced param to be false by default.
|
@adampalay @awaisdar001 comments addressed. |
| """ | ||
| Returns True if course is self-paced, False otherwise. | ||
| """ | ||
| return course and course.self_paced and SelfPacedConfiguration.current().enabled |
There was a problem hiding this comment.
Can you check if there is any table in mysql where this mapping is added? I mean where we add information about course if it is self paced?
There was a problem hiding this comment.
I have checked but there is no such SelfPaced to Course mapping table in MySQL.
|
👍 |
…cheduled-content Fix self paced course content visiblity
mushtaqak
deleted the
mushtaq/ECOM-3615-fix-self-paced-scheduled-content
branch
ECOM-3615
Fix self-paced course displaying "published, not yet released" issue