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

fix(user_report_viewed): Stops errors where course ID is 0. #527

Merged
merged 9 commits into from
May 3, 2019

Conversation

garemoko
Copy link
Contributor

@garemoko garemoko commented May 2, 2019

Description

Known Issues Not Fixed By This PR (Out of Scope)
If the course if not defined, the lang code defaults to "en". This is the same default that is already used as a fallback in case the lang code of a course is not valid, so I don't feel bad about using this default again here. Ideally we should use the site default as the fallback, but that should be fixed everywhere and is a separate issue.

Related Issues

PR Type

  • Fix

@garemoko
Copy link
Contributor Author

garemoko commented May 2, 2019

@ryansmith94 This is ready for review.

@ryasmi ryasmi added the fix label May 2, 2019
@ryasmi ryasmi self-assigned this May 2, 2019
@ryasmi ryasmi self-requested a review May 2, 2019 12:55

return [
'id' => $config['app_url'].'/mod/forum/user.php?id='.$user->id.'&course='.$course->id,
$activity = [
'definition' => [
'type' => 'http://id.tincanapi.com/activitytype/user-profile',
'name' => [
$courselang => 'forum posts of '.utils\get_full_name($user),
Copy link
Member

@ryasmi ryasmi May 2, 2019

Choose a reason for hiding this comment

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

Since we're using the related user ID below for the activity ID, does that mean we should use the related user here? Same comment for the extension below too.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, definitely. Well spotted!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In which case i probably shouldn't have modified the user part of this file, but instead should have just changed the user that is passed in by user_report_viewed.php

Copy link
Member

Choose a reason for hiding this comment

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

Sweet. Yeah good idea that probably makes the most sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ryansmith94 this is resolved.

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.

Noice 👍

@ryasmi ryasmi changed the title add support for course 0 in forum user report events fix(user_report_viewed): Stops errors where course ID is 0. May 3, 2019
@ryasmi ryasmi merged commit 99b6c55 into xAPI-vle:master May 3, 2019
@HT2Bot
Copy link
Member

HT2Bot commented May 3, 2019

🎉 This PR is included in version 4.2.5 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@HT2Bot HT2Bot added the released label May 3, 2019
@garemoko garemoko deleted the user_report_viewed_fix branch May 3, 2019 11:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

user_report_viewed event course id is sometimes 0, but this causes error
3 participants