Skip to content

feat: sync course tabs #526

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

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft

feat: sync course tabs #526

wants to merge 7 commits into from

Conversation

asadali145
Copy link
Contributor

What are the relevant tickets?

https://github.com/mitodl/hq/issues/7546

Description (What does it do?)

Sync static tabs with the target courses.

Screenshots (if appropriate):

  • Desktop screenshots
  • Mobile width screenshots

How can this be tested?

Additional Context

Copy link
Contributor

@arslanashraf7 arslanashraf7 left a comment

Choose a reason for hiding this comment

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

I skimmed through the PR and left a few comments.

Comment on lines 94 to 121
"""
Listen for the course static tab changes and trigger static tab sync
"""
xblock_info = kwargs.get("xblock_info")
if not xblock_info or not isinstance(xblock_info, XBlockData):
log.error("Received null or incorrect data for event")
return

if xblock_info.block_type != "static_tab":
return

course_key = xblock_info.usage_key.course_key
if not CourseSyncOrganization.objects.filter(
organization=course_key.org, is_active=True
).exists():
return

course_sync_mappings = CourseSyncMapping.objects.filter(
source_course=course_key, is_active=True
)
if not course_sync_mappings:
return

for course_sync_mapping in course_sync_mappings:
sync_course_static_tabs.delay(
str(course_sync_mapping.source_course),
str(course_sync_mapping.target_course),
)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can probably do some refactoring of the order of the conditions here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you share what's on your mind? I kept the order intentionally because there would be many signals, and I wanted to avoid the database queries.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I see, makes sense then.


@receiver(XBLOCK_CREATED)
@receiver(XBLOCK_DELETED)
@receiver(XBLOCK_UPDATED)
Copy link
Contributor

Choose a reason for hiding this comment

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

is this raised if we change anything in static pages?

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we listen for the xBlock published signal? I can think of one reason why that's not here. We already have a signal for the course publish and it would make this duplicate? In any case, we would want to avoid duplicate signals. At the moment I am unsure of these will collide with each other or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are raised when we create, change, or delete the static pages. Static pages have no relation to course publishing.

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