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

Remove WorkshopAutoenrolledApplication class #26789

Merged
merged 16 commits into from
Jan 30, 2019

Conversation

clareconstantine
Copy link

Previously, teacher and facilitator applications both inherited from WorkshopAutoenrolledApplication, which inherited from ApplicationBase. We no longer have the concept of an autoenrollment, so I removed all code relating to autoenrolling and moved the remaining shared functionality into a module called Pd::Application::PdWorkshopHelper (open to better names - this module does caching as well) that both TeacherApplicationBase and FacilitatorApplicationBase include.

Copy link
Contributor

@islemaster islemaster left a comment

Choose a reason for hiding this comment

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

Great cleanup! I think most of my nits here are existing issues that just got moved to a new place, but I do want to make sure we preserve test coverage of the default workshop business logic.

Copy link
Contributor

@islemaster islemaster left a comment

Choose a reason for hiding this comment

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

👍 Yay removing dead code!

dashboard/app/helpers/pd/application/pd_workshop_helper.rb Outdated Show resolved Hide resolved
include Pd::FacilitatorCommonApplicationConstants

serialized_attrs %w(
pd_workshop_id
Copy link
Contributor

Choose a reason for hiding this comment

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

should this just go on PdWorkshopHelper?

Copy link
Author

Choose a reason for hiding this comment

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

Having this as a serialized attribute means that the models that use our helper would have to have a properties column and include (or the module could include) SerializedProperties. does that make this helper depend too much on the models? I am leaning towards keeping this in the models.

serialized_attrs %w(
auto_assigned_enrollment_id
)

# @override
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume that this code will never get executed in real life again because it's 1819? If it is dead code, should we remove it completely instead of updating it? (I guess "removing" could be deleting the route configs so request won't hit this code path.)

Copy link
Author

Choose a reason for hiding this comment

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

Since instances of these models still exist in our database, and at some point we plan to allow partners to download CSVs of their past applications, I want to preserve the data, including the associations between these models.

Currently, no routes are loading these applications.

@clareconstantine clareconstantine merged commit f915391 into staging Jan 30, 2019
@clareconstantine clareconstantine deleted the remove-ws-autoenroll-app branch February 17, 2019 21:11
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.

None yet

4 participants