Skip to content

Fix for optional lineitem props being included in deep link return#170

Open
snake wants to merge 1 commit intopackbackbooks:masterfrom
snake:fix-deeplink-lineitem-return
Open

Fix for optional lineitem props being included in deep link return#170
snake wants to merge 1 commit intopackbackbooks:masterfrom
snake:fix-deeplink-lineitem-return

Conversation

@snake
Copy link

@snake snake commented Jan 28, 2026

Summary of Changes

Fixes #169

Testing

Unit test coverage is included.

  • [Y] I have added automated tests for my changes
  • [Y] I ran composer test before opening this PR
  • [Y] I ran composer lint-fix before opening this PR

…eturn as nulls

These are optional, and should be omitted if not set, not sent as null.
@snake snake requested a review from dbhynds as a code owner January 28, 2026 07:43
@coveralls
Copy link

Coverage Status

coverage: 98.655%. remained the same
when pulling 185db34 on snake:fix-deeplink-lineitem-return
into 67a1524 on packbackbooks:master.

'scoreMaximum' => $this->line_item->getScoreMaximum(),
'label' => $this->line_item->getLabel(),
'resourceId' => $this->line_item->getResourceId(),
'tag' => $this->line_item->getTag(),
Copy link
Author

@snake snake Jan 28, 2026

Choose a reason for hiding this comment

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

Had a thought that we could probably just use $this->line_item->toArray() here, instead of manually assembling it. Happy to change patch if that's the direction you want to go.

Edit: thinking about this a bit more, that is probably not a good fit, given LtiLineItem represents the full set of props, whereas deep linking content items only include a subset.

@AsiaH-dev AsiaH-dev self-assigned this Feb 27, 2026
Copy link

@AsiaH-dev AsiaH-dev left a comment

Choose a reason for hiding this comment

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

This looks good to me! The changes look safe & reasonable. I tested them with our project and deep linking still works as expected.

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.

Optional line item props cannot be omitted during deep linking return

3 participants