Skip to content
This repository was archived by the owner on Feb 27, 2024. It is now read-only.

[MCKIN-7759] Remove usage of CourseAggregatedMetaData since it is no longer in use.#208

Merged
xitij2000 merged 3 commits intomasterfrom
kshitij/mckin-7759
Mar 20, 2019
Merged

[MCKIN-7759] Remove usage of CourseAggregatedMetaData since it is no longer in use.#208
xitij2000 merged 3 commits intomasterfrom
kshitij/mckin-7759

Conversation

@xitij2000
Copy link
Contributor

@xitij2000 xitij2000 commented Mar 6, 2019

The data CourseAggregatedMetaData was storing is now provided by the completion aggregator, code using it and signals keeping it in sync can be removed.

serializer_context.update({
'course_id': self.course_key,
'default_fields': default_fields,
'course_meta_data': self.course_meta_data,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This isn't being used the serializer any more so remove it.

"""
SignalHandler.course_published.connect(
listener_in_course_metadata, dispatch_uid='course_metadata'
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since course metatdata is no longer being used, we no longer need to keep it up to date.

url(r'^organizations/*', include('edx_solutions_organizations.urls')),
url(r'^mobile/v1/', include('edx_solutions_api_integration.mobile_api.urls')),
url(r'^imports/*', include('edx_solutions_api_integration.imports.urls')),
url(r'^courses_metadata/', include('course_metadata.urls')),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could not see this API being used by Apros.

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 connected to the old progress code? I'm not familiar with the CourseAggregatedMetadata model or this api. Do you know what it was intended to do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added it back since it wasn't really in scope. I was just cleaning up some unused code while I was at it, but I don't want to blow up the scope and review time too much here.

@xitij2000 xitij2000 force-pushed the kshitij/mckin-7759 branch from 3f73e2e to 0f64f5f Compare March 18, 2019 12:41
Copy link

@xirdneh xirdneh left a comment

Choose a reason for hiding this comment

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

👍

  • I tested this:
    • Ran this in local Vagrant devstack.
  • I read through the code
  • [ ] I checked for accessibility issues
  • [ ] Includes documentation
  • [ ] I made sure any change in configuration variables is reflected in the corresponding
    client's configuration-secure repository.

@xitij2000 xitij2000 force-pushed the kshitij/mckin-7759 branch from 7ad6d30 to 28705c0 Compare March 20, 2019 09:22
@xitij2000 xitij2000 merged commit cb102cc into master Mar 20, 2019
@xitij2000 xitij2000 deleted the kshitij/mckin-7759 branch March 20, 2019 09:25
xitij2000 added a commit that referenced this pull request Apr 10, 2019
naeem91 pushed a commit that referenced this pull request Apr 11, 2019
naeem91 pushed a commit that referenced this pull request Apr 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants