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

HFP-2854: FitB reports updated to handle alternatives. #7

Merged
merged 2 commits into from
May 21, 2021

Conversation

jwalits
Copy link
Contributor

@jwalits jwalits commented Dec 24, 2019

PR created to address: https://h5ptechnology.atlassian.net/browse/HFP-2854****
FitB Report updated to support "https://h5p.org/x-api/alternatives" extension

Copy link
Member

@thomasmars thomasmars left a comment

Choose a reason for hiding this comment

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

I get a php error when running this:
Notice: Trying to access array offset on value of type bool in /var/www/html/mod/hvp/reporting/type-processors/fill-in-processor.class.php on line.
Also, this will not work for older versions of fill in the blanks, the reporting has to be backwards-compatible with older versions, so those reports can still be generated.
Finally, there are other content types that use this report processor, any content type that is a "fill-in" content type. These will no longer work with these changes, because they may not use he case-sensitivity or alternatives extensions.

@jwalits
Copy link
Contributor Author

jwalits commented Apr 15, 2020

Thanks for the feedback @thomasmars,

I have updated the code to have it backwards compatible. In my testing so far, I haven't seen that PHP Notice you mentioned?

If you could please review and provide any feedback, it would be much appreciated.

Thanks,
Jay

Copy link
Member

@thomasmars thomasmars left a comment

Choose a reason for hiding this comment

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

Looks much better.
The buildReportOutput function expects $caseMatters to be an array:

$report = $this->buildReportOutput($description,
$processedCRPs,
$processedResponse,
$caseMatters['caseSensitive']
);

This change makes the value a boolean, which causes the notice. You can enable Moodle debugging by following: https://docs.moodle.org/38/en/Debugging#Enabling_debugging

@jwalits jwalits force-pushed the master_HFP2854 branch 2 times, most recently from a0031a4 to a1cb3d8 Compare April 17, 2020 08:08
@jwalits
Copy link
Contributor Author

jwalits commented Apr 17, 2020

Thanks @thomasmars,

I have made that change. Appreciate your feedback.

Regards,
Jay

@jwalits
Copy link
Contributor Author

jwalits commented Apr 30, 2020

Hey @thomasmars,

Hope you are well. When you have a chance, would you be able to look at the updated patch?

Regards,
Jay

Copy link
Member

@thomasmars thomasmars left a comment

Choose a reason for hiding this comment

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

Looks good, we'll have to do some regression testing before merging it in. Thanks

@jwalits
Copy link
Contributor Author

jwalits commented May 8, 2020

Hey @thomasmars,

Thanks for the update, looking forward to seeing this merged into main repo.

Regards,
Jay

@jwalits
Copy link
Contributor Author

jwalits commented May 18, 2020

Hi @thomasmars,

Hope you are well? Do you have an ETA on when you think the changes will be merged into the main code?

Regards,
Jay

@jwalits
Copy link
Contributor Author

jwalits commented Jun 25, 2020

Hi,

Any update on the regression testing with this one? Do you have an ETA when you expect the changes to be merged in?

Regards,
Jay

@thomasmars
Copy link
Member

Sorry this is taking a while, we usually do pull requests and regression testing when we're about to release a new plugin version, but we've been swamped with building the content hub lately https://h5p.org/oer-hub-coming, so there's been no new releases. I have no ETA, but I'm pretty sure we'll be releasing a new version soon.

@golenkovm
Copy link

Hi @thomasmars
Can you please let me know if this fix will be included into a new version?
And maybe now you know when it's going to be released (eg roughly in a week, in a month, in a year)?

Thanks,
Mikhail

@Urpokarhu1
Copy link

Hello,

Any progress on this?

@Urpokarhu1
Copy link

Urpokarhu1 commented Oct 19, 2020

Hello,

When will this be available in mod_hvp? @thomasmars

@thomasmars
Copy link
Member

With the next release.

@bozicm
Copy link

bozicm commented Mar 22, 2021

Hi, we'd try to fork with this patch but unfortunately we don't have any experience with H5P code. Is the code usable then as such? Or is this conflict a show stopper?

@golenkovm
Copy link

Hey @thomasmars
The conflict is resolved now.
Could you please have a look and advise when this pull request might be merged in?

Cheers,
Mikhail

Copy link
Member

@thomasmars thomasmars left a comment

Choose a reason for hiding this comment

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

Thanks, could you change the private static to constants instead, unless there is a specific reason for having them like this ?

@jwalits
Copy link
Contributor Author

jwalits commented May 21, 2021

Hey @thomasmars

Thanks for the feedbak. There was no specific reason for the static variables. I have updated the code to make them constants.

Regards,
Jwalit

@thomasmars thomasmars merged commit 0968db5 into h5p:master May 21, 2021
@thomasmars
Copy link
Member

Thanks :)

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

5 participants