ndunand / moodle-qtype_matrix

Source code of https://moodle.org/plugins/qtype_matrix
2 stars 10 forks source link

Rework code #58

Closed cli-ish closed 1 year ago

cli-ish commented 1 year ago

This PR contains:

  1. remove unused functions
  2. solved all code guidelines flaws (via codechecker)
  3. fixed restore error
  4. added class autoloading
  5. added amd module instead of dnd.js
  6. added new grading method (difference)
  7. added form filter for difference method
  8. code simplified
  9. unified coding structure. (removed iterator stuff and other complex structures not needed).

I would suggest that: .github/workflows/moodle-plugin-ci.yml

      #      - name: Moodle Code Checker
      #        if: ${{ always() }}
      #        run: moodle-plugin-ci codechecker --max-warnings 0

from now on runs on each commit to avoid diviation from the moodle coding style.

ndunand commented 1 year ago

Thanks for sharing! We're on the way to have a closer look at this.

@cli-ish could you share a bit about where all this work comes from? Was this part of a project, or of an update at your University? Also, did you use this reworked code in a production environment already?

Thanks again, Nicolas

ndunand commented 1 year ago

Hi @CamilleTrd we should talk about this.

cli-ish commented 1 year ago

Hi @ndunand,

This work is made for a university which wanted a new grading method (difference). To make this project a bit easier to maintain we also applied basic moodle guidlines. This PR was not tested fully with all given features, but we played around a bit and it all seems to work. There where no big changes made to the code logic except for the restore/difference-method. But i would recomend testing by someone who knows how all of these things should behave.

We will have additional feedback on the changes made in about 2 weeks if it is used. This PR can remain on hold until thats cleared :)

PS: i noticed that the merge with the latest version seems to have mixed up the version.php version (2022121604 should be 2023010300) and https://github.com/ndunand/moodle-qtype_matrix/blob/1148b2dc4f7083bf132c164fb44b3945a36aca4f/db/upgrade.php#L67-L89 should be set to the current version (2023010300). This should be fixed after PR.

Kind regards Vincent

ndunand commented 1 year ago

Hi @cli-ish ,

Thanks for the clarification and explaining your usage context. I reiterate out thanks for sharing this. We'll look into this in the coming days and hopefully amend/approve the merge.

yaakovopenapp commented 1 year ago

Hello We see a patch that works properly It should be noted that in the version 4.1 model

Before the final submission of a partially answered question, an alert is displayed that the question was not answered And you should change the alert to say that the question was partially answered

Screenshot from 2023-03-19 12-49-05

ndunand commented 1 year ago

Hello @cli-ish , I finally took the first steps to test this on our test Moodle 4.1.2 (Build: 20230313).

First problem, we can't create a new Matrix question for the following error and debug message display (truncated). The form displays afterwards but the items and answers table does not.

QuickForm Error: nonexistent html element Element 'tagsheader' does not exist in HTML_QuickForm::getElement()

Screenshot 2023-03-22 at 11 21 41
cli-ish commented 1 year ago

@ndunand

The Issue was that I used the function $this->_form->getElement('tagsheader') to fetch the Element and did not notice that it drops and error if the element is not present. I now use the $this->_form->_elementIndex['tagsheader'] to ensure that this form has the tagsheader field. (This was always present at my end, since i tested with tags enabled).

We are also in the process to test the PR.

cli-ish commented 1 year ago

@yaakovopenapp This could potential relate to question.php -> is_complete_response I will take a look into it.

cli-ish commented 1 year ago

@yaakovopenapp The commit 642f3ec should fix the issue you found. We now allow partial questions with all/difference method to be "complete" when atleast one row is answered.

cli-ish commented 1 year ago

@ndunand We have tested the PR and found no other bugs, so we can say from our side that it can be merged. Or is there something missing?

relecand commented 1 year ago

Hello We see a patch that works properly It should be noted that in the version 4.1 model

Before the final submission of a partially answered question, an alert is displayed that the question was not answered And you should change the alert to say that the question was partially answered

Screenshot from 2023-03-19 12-49-05

It works in Moodle 3.9 until 4.1 (with at least PHP 7.4). Moodle 4.2 not testet yet.

relecand commented 1 year ago

It will also fix this issue: https://github.com/ndunand/moodle-qtype_matrix/issues/27

NicoAlexH commented 1 year ago

@cli-ish @relecand We have tested the PR on our side as well, we found a small database upgrade issue but otherwise everything seems to be working fine. We're merging it and adding the tiny fix for the upgrade issue. Thank you very much for your work !

ndunand commented 1 year ago

@cli-ish thanks for sharing your contribution!