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

Enforce plc stage conventions #13949

Merged
merged 3 commits into from Mar 23, 2017
Merged

Conversation

mehalshah
Copy link
Contributor

@mehalshah mehalshah commented Mar 22, 2017

For PLC scripts, either all stages should have flex categories or none of them should. Additionally, flex categories should be grouped together.

…dules are grouped together in the correct way - content modules with content modules, practice modules with practice modules
@mehalshah mehalshah requested a review from Hamms March 22, 2017 20:52
Copy link
Contributor

@Hamms Hamms left a comment

Choose a reason for hiding this comment

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

LGTM modulo a couple questions

learning_module_types = stages.map(&:flex_category)

unless learning_module_types.uniq == [nil] || Set.new(learning_module_types) <= Set.new(Plc::LearningModule::MODULE_TYPES)
raise "#{name}, either all stages must have flex categories that correspond to learning module types, or none of them should have categories"
Copy link
Contributor

Choose a reason for hiding this comment

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

any reason why we're not raising a more-specific RecordInvalid error here and below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

RecordInvalid happens when trying to persist an object to the DB. That's not happening here, so it's not really appropriate to raise that exception

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually I might be able to just raise this

raise "#{name}, either all stages must have flex categories that correspond to learning module types, or none of them should have categories"
end

unless learning_module_types.compact.empty? || learning_module_types.chunk { |x| x }.map(&:first) == learning_module_types.uniq
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't the learning_module_types.compact.empty? case redundant 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.

No - if the pd course has no flex categories, then learning_modules_types is [nil, nil, nil] in which case this is needed

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 was falsely assuming that learning_module_types.chunk { |x| x }.map(&:first) would give us [nil] in that situation

end
end

test 'Cannot setup script where some of the stages are missing flex categories' do
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like ya forgot to update the name of this test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will change

@mehalshah mehalshah merged commit 96b8349 into staging Mar 23, 2017
@davidsbailey davidsbailey deleted the enforce_plc_stage_conventions branch August 8, 2017 17:22
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

2 participants