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: Adds id number in object extension for courses and activities to close #230. #272

Merged
merged 2 commits into from
Sep 26, 2018

Conversation

BrendanHalley
Copy link
Contributor

Description
Adds id number in object extension for courses and activities under the extension https://w3id.org/learning-analytics/learning-management-system/external-id.

There is a config option (disabled by default) for this extension to keep consistency with the shortid extension.

Related Issues
Related issue #230

PR Type
Enhancement

}

if (utils\is_enabled_config($config, 'send_course_and_module_idnumber')) {
$courseidnumber = property_exists($course, 'idnumber') ? $course->idnumber : null;
Copy link
Member

Choose a reason for hiding this comment

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

@davidpesce wonder if we should have the same check for shortname above, but probably ok.

Copy link
Member

@ryasmi ryasmi left a comment

Choose a reason for hiding this comment

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

Thanks @trewq, bonus points for the correct PR title and commit format 🌟

],
];

if (utils\is_enabled_config($config, 'send_short_course_id')) {
Copy link
Member

Choose a reason for hiding this comment

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

Usually prefer to avoid mutations because it becomes quite hard to read as the size of the code grows, but that is quite painful in PHP (usually work in JS where this is easier to avoid). Happy with this though, just thought I'd explain why it wasn't like that before, just a personal habit.

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 prefer to avoid doing it this way too and it's definitely won't be suitable as the codebase grows.

Handling extensions moving forward will probably need to be split off into its own utils. This will allow for much easier additions and allow the possibility for custom extension names to be set in the config area. As I think the extension names are too specific but making them more generic would just be confusing.

@@ -77,6 +77,7 @@ protected function get_transformer_config() {
'send_mbox' => false,
'send_response_choices' => false,
'send_short_course_id' => false,
'send_course_and_module_idnumber' => false,
Copy link
Member

Choose a reason for hiding this comment

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

@davidpesce this reminds me that we should probably have some tests that check these config values still work in the plugin when they're enabled.

@ryasmi ryasmi merged commit bdaa16b into xAPI-vle:master Sep 26, 2018
@HT2Bot
Copy link
Member

HT2Bot commented Sep 26, 2018

🎉 This PR is included in version 3.18.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@BrendanHalley
Copy link
Contributor Author

Thanks @ryansmith94. I'm hoping to be able to contribute more.

@ryasmi
Copy link
Member

ryasmi commented Sep 26, 2018

You're welcome @trewq, that'd be awesome if you can 👍

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.

4 participants