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

feat: new COURSE_CATALOG_INFO_CHANGED signal for course_authoring #81

Merged
merged 9 commits into from Jul 25, 2022

Conversation

rgraber
Copy link
Contributor

@rgraber rgraber commented Jul 21, 2022

Description:
Adds a new OpenEdxPublicSignal to describe the changing of the catalog information for a course

Issue: #72

Reviewers:

  • tag reviewer
  • tag reviewer

Merge checklist:

  • All reviewers approved
  • CI build is green
  • Version bumped
  • Changelog record added
  • Documentation updated (not only docstrings)
  • Commits are squashed

Post merge:

  • Create a tag
  • Check new version is pushed to PyPI after tag-triggered build is
    finished.
  • Delete working branch (if not needed anymore)

@rgraber rgraber changed the title refactor!: refactor avro_attrs_bridge into utility methods feat: new COURSE_CATALOG_INFO_CHANGED event for course_authoring Jul 21, 2022
@rgraber rgraber changed the title feat: new COURSE_CATALOG_INFO_CHANGED event for course_authoring feat: new COURSE_CATALOG_INFO_CHANGED signal for course_authoring Jul 21, 2022
@rgraber rgraber force-pushed the timmc/course-publish-event branch from bd987fc to 60a4b98 Compare July 21, 2022 18:03
@rgraber rgraber marked this pull request as ready for review July 21, 2022 18:08
Copy link
Contributor

@robrap robrap left a comment

Choose a reason for hiding this comment

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

Nice. Lots of minor questions/comments that hopefully aren't too controversial.

CHANGELOG.rst Outdated Show resolved Hide resolved
CHANGELOG.rst Outdated Show resolved Hide resolved
openedx_events/__init__.py Outdated Show resolved Hide resolved
openedx_events/content_authoring/__init__.py Outdated Show resolved Hide resolved
openedx_events/content_authoring/data.py Outdated Show resolved Hide resolved
openedx_events/content_authoring/data.py Outdated Show resolved Hide resolved
openedx_events/content_authoring/data.py Outdated Show resolved Hide resolved
openedx_events/content_authoring/signals.py Outdated Show resolved Hide resolved
openedx_events/event_bus/avro/tests/test_avro.py Outdated Show resolved Hide resolved
Copy link

@ormsbee ormsbee left a comment

Choose a reason for hiding this comment

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

Some questions (mostly because I'm not as familiar with this data as I should be).

openedx_events/content_authoring/data.py Outdated Show resolved Hide resolved
openedx_events/content_authoring/data.py Outdated Show resolved Hide resolved
openedx_events/content_authoring/data.py Show resolved Hide resolved
openedx_events/content_authoring/data.py Outdated Show resolved Hide resolved
@rgraber
Copy link
Contributor Author

rgraber commented Jul 22, 2022

@ormsbee @robrap
As I understand it, the problem is that we don't send media across in a consistent format in the LMS API call that we're trying to replace, nor does Discovery save in a consistent format. For course_video, we send both relative URI/path and absolute URI/URL. For banner_image and course_image we just send a path, and for image we send over three full URLs for raw, small, and large.

After some discussion, I think our options are

  1. Copy the existing fields exactly as they are from the LMS response and just acknowledge that they are weird and inconsistent. We could still fix the field names to be a little clearer.
  2. Create an ImageData attrs-class with path and URL fields. Use this class for video, banner_image, and course_image. Then, create a SizedImageData class that is just three ImageData attributes, one each for raw, small, and large. Make any bits we can't always populate optional.
  3. Always send just the URL and let clients deal with extracting the path.

Notes:
For (2), we might end up with a lot of optional fields which isn't great for consumers
For (3), we're not sure if we can get the absolute uri from within Studio

@robrap 's order of preference is (3) then (1). This preference for (3) assumes we are able to get absolute uris from within Studio without too much headache and then strip down to a relative path in Discovery with a similarly small headache. He really doesn't like (2) because of the confusing nature of a bunch of optional fields.

My order of preference is (1) then (3), though it's not a terribly strong preference and is mostly in service of getting this out quicker. I agree (2) has the potential to be very confusing and doesn't save that much work over (3). If (3) is quick, then I'm all for it, I'm just not sure how long it will take to determine if it's quick.

I think both of us agree we can at least start with 1, and then when implementing the actual event in Studio determine if we could do 3 without much more effort.

@rgraber
Copy link
Contributor Author

rgraber commented Jul 25, 2022

As far as I can tell, while it used to be possible to edit media in Studio, it is not possible any more. This would suggest all the various media fields sent over from the lms to discovery are legacy. We could turn on some temporary logging in discovery to see if it ever actually updates course media as a result of RCM to support this hypothesis.

@rgraber
Copy link
Contributor Author

rgraber commented Jul 25, 2022

For posterity, the decision was to drop all the media fields since they are unused by course-discovery. See the deprecation ticket at openedx/public-engineering#160 .

Copy link
Contributor

@robrap robrap left a comment

Choose a reason for hiding this comment

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

I'm basically all set, other than resolving Dave's comments. Thank you.

openedx_events/content_authoring/data.py Show resolved Hide resolved
@rgraber rgraber merged commit 98f99ea into main Jul 25, 2022
@rgraber rgraber deleted the timmc/course-publish-event branch July 25, 2022 19:44
Comment on lines +44 to +45
org (str): course organization identifier
number (str): course number
Copy link
Contributor

Choose a reason for hiding this comment

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

[nit] org/number are in a different order here than in the attributes.

schedule_data (CourseScheduleData): scheduling information for the course
short_description (str): one- or two-sentence course description (optional)
effort (str): estimated level of effort in hours per week (optional). Kept as a str to align with the lms model.
hidden (bool): whether the course is hidden from search (optional)
Copy link
Contributor

Choose a reason for hiding this comment

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

[nit] hidden uses (optional) on a bool with a default, but invitation_only does not.

Suggested change
hidden (bool): whether the course is hidden from search (optional)
hidden (bool): whether the course is hidden from search (optional)

@rgraber rgraber added the event-bus Work related to the Event Bus. label Jul 25, 2022
@rgraber
Copy link
Contributor Author

rgraber commented Sep 14, 2022

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
event-bus Work related to the Event Bus.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants