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

[Instrument API] expand saving functionality to include Candidate age calculations and scoring #7485

Merged
merged 3 commits into from
Jun 16, 2021

Conversation

ridz1208
Copy link
Collaborator

Brief summary of changes

Currently the API only calls the _save() function which stores the data sent to it in the appropriate instrument. That workflow is not symmetrical to saving on the front end which computes the age and scores.

This PR is not making it 100% symmetrical either since validation is not being added but it gets closer to the target

Testing instructions (if applicable)

  1. try saving an instrument using the API and see if scoring and candidate age calculations are done

Link(s) to related issue(s)

@@ -171,7 +171,8 @@ class Instrument extends Endpoint implements \LORIS\Middleware\ETagCalculator
try {
$instrumentname = $this->_instrument->testName;
$this->_instrument->clearInstrument();
$this->_instrument->_save($data[$instrumentname]);
$this->_instrument->_saveValues($data[$instrumentname]);
$this->_instrument->score();
Copy link
Collaborator

Choose a reason for hiding this comment

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

The only thing missing from the validate code path of the save() function seems to be:

// determine the data entry completion status, and store that in
// the database
$dataEntryCompletionStatus = $this->_determineDataEntryCompletionStatus();
$this->_setDataEntryCompletionStatus(
    $dataEntryCompletionStatus
);

why not just add that?

You could also try moving the code into a final public class method and calling that from both save and the api, to ensure that they're always doing the same thing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

can I just move the data entry completion status logic to save values instead ? or you prefer not ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I looked into the IBIS and CCNA repositories and on IBIS there's 19 instrument classes that override _saveValues, mostly to implement instruments that have an uploader. (I think it might be the only way to do it?)

CCNA, on the other hand, has a bunch of scripts that call _saveValues but no instruments that override it.

It's probably safer to keep it where it is to ensure that it doesn't get missed.

@driusan
Copy link
Collaborator

driusan commented Jun 16, 2021

@xlecours can you look at this too?

@ridz1208
Copy link
Collaborator Author

@driusan changes done.

just curious why you wanted to do it final ?

@driusan
Copy link
Collaborator

driusan commented Jun 16, 2021

I meant including the _saveValues and score parts. I just realized looking at the latest change that that's not possible because of the $this->form->process wrapper around _saveValues to extract the values. The reason for it being final is because (if those 2 calls were included) it's an internal helper that's only public so that the API (which is in a different class/module) can call it to ensure the same behaviour.

i'm not opposed to the updateDataEntryCompletionStatus being moved out. It's a little cleaner.. but you're right there's no reason for that to be final on its own.

@ridz1208
Copy link
Collaborator Author

Ohhh I get it. let me remove the final and it'll be ready

@driusan driusan merged commit 421c96e into aces:23.0-release Jun 16, 2021
@ridz1208 ridz1208 added this to the 23.0.4 milestone Jun 16, 2021
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

3 participants