packbackbooks / lti-1-3-php-library

A library used for building IMS-certified LTI 1.3 tool providers in PHP.
Apache License 2.0
39 stars 25 forks source link

Clean up logic for putting a grade to a line item, add helper functions to extract default line item, and avoid extra line item lookups when not needed #44

Closed dbhynds closed 2 years ago

dbhynds commented 2 years ago

Summary of Changes

This PR makes a few changes from the original PR (https://github.com/packbackbooks/lti-1-3-php-library/pull/42). These changes are primarily linting and the recommendations. Refer to the other PR for a more detailed discussion of why these changes have been made. (I created a separate branch myself so I could easily make changes and test it locally).

Testing

dbhynds commented 2 years ago

@zlayaAvocado In terms of testing, this PR shows these changes implemented in our platform and is passing in CI. You could branch off and test it manually if you want, but our gradebook sync E2E tests all pass.

dbhynds commented 2 years ago

@mattwright Do these changes look right to you? If so, I think this is ready to merge.

mattwright commented 2 years ago

@dbhynds

The logic in ensureLineItemExists() looks good -- much cleaner. My only thought with that method is that it may be better as a private.

Additionally, I wonder if we want to use that now in the getGrades() area to sync up the behavior of these symmetrical methods (put/get grades). We could ditch the confusing logic there in favor of ensureLineItemExists().

    public function getGrades(LtiLineitem $lineitem = null)
    {
        $lineitem = $this->ensureLineItemExists($lineitem);
        $resultsUrl = $lineitem->getId();

        // Place '/results' before url params
        $pos = strpos($resultsUrl, '?');
        $resultsUrl = $pos === false ? $resultsUrl.'/results' : substr_replace($resultsUrl, '/results', $pos, 0);

        $request = new ServiceRequest(LtiServiceConnector::METHOD_GET, $resultsUrl);
        $request->setAccept(static::CONTENTTYPE_RESULTCONTAINER);
        $scores = $this->makeServiceRequest($request);

        return $scores['body'];
    }

It changes the behavior a bit since the original getGrades() method doesn't create the default line item like putGrade() does. Again, I am not sure why we should even be creating a default grade item with a presumed 100 max score in putGrade(), but since the original library did it, I guess we can just leave it.

Thoughts?