chamilo / chamilo-lms

Chamilo is a learning management system focused on ease of use and accessibility
https://chamilo.org
GNU General Public License v3.0
798 stars 480 forks source link

Learning paths: prerequisite on test with random questions shows wrong maximum #3612

Closed ywarnier closed 7 months ago

ywarnier commented 3 years ago

This one is a bit difficult to reproduce:

The issue here is that the value in the "maximum" column is not the real maximum you can get from the test. You can only get 10. The "minimum" column works fine.

Make sure the "maximum" shows "10" (in this case). In other words, make sure the "maximum" column shows the maximum of the test based on its configuration, not just the sum of scores of all questions that exist in the test. Somehow, the "minimum" column already has the right calculation. Re-use that one.

carlangas159 commented 3 years ago

Hi @ywarnier

I have tried to replicate the problem and I must say that it does not appear that way.

imagen I asked 4 questions so that the sum was 5, divided into 2 categories. imagen Then I established the prerequisite and it shows me at most 5, since there are 4 questions where 3 have a value of 1 and only one of 2.

On the other hand, do a set of exercises with 4 questions, all with a value of 5. Based on the same logic of 2 categories.

imagen Where the displayed value is correct. imagen For this case, there are 4 questions with a value of 5 each, which gives a maximum of 20

For now, could you give me more information about this case since I could not replicate it?

ywarnier commented 3 years ago

@carlangas159 did you select a random selection of questions, with less than 4 questions selected?

carlangas159 commented 3 years ago

hi @ywarnier

In deed, y just a make a new test with random category imagen

with random question and the max score is set to 3 imagen

I have also placed 2 questions by categories and I do not get the error.

imagen

Both were with random categories.

imagen

peterdanielmyers commented 3 years ago

This issue is miss-labelled as an enhancement. It's actually a bug, and a pretty serious one.

I'm having it.

In my learning path here, I get this pop up warning that does not allow me to enter "5" for this test. But the maximum score for the test really is 5, not 3:

Screenshot 2021-06-18 at 10 55 25 PM

peterdanielmyers commented 3 years ago

I've now worked out how to recreate this, and am pinning down where the problem is.

  1. Create a test, and add a few questions so the max score is, say, 5.
  2. If you inspect the SQL table c_lp_item, the column max_score will correctly list 5 for this test.
  3. Create a learning path, and add the test to the learning path.
  4. Create something else in the learning path that the test will be a prerequisite for.
  5. Now click on the prerequisites button in the edit learning path. Here's a screenshot: Screenshot 2021-06-19 at 6 23 13 PM
  6. Loading this page writes to the SQL of the prerequisite items. It should not do this. Now if you inspect the SQL table c_lp_item, the column max_score will incorrectly list a lower number for the prerequisite test, in my case 3.

I don't think tests need to even be set as prerequisites. They just need to appear earlier in the learning path when you load the "Add/edit prerequisites" screen.

peterdanielmyers commented 3 years ago

I've narrowed down the problem further.

I should note that I'm using the latest release of Chamilo, but I'll reference the code in the current repo.

In the Learnpath class: https://github.com/chamilo/chamilo-lms/blob/master/public/main/lp/learnpath.class.php

The member function display_item_prerequisites_form() is called to build the form when viewing prerequisites in the learning path.

In that function, there is an if statement starting on ln 6382 and hte first few lines of code read like this:

                if (TOOL_QUIZ == $type) {
                    // lets update max_score Tests information depending of the Tests Advanced properties
                    $exercise = new Exercise($course_id);
                    /** @var CLpItem $itemEntity */
                    $itemEntity = $lpItemRepo->find($itemId);
                    $exercise->read($item['path']);
                    $itemEntity->setMaxScore($exercise->get_max_score());
                    $em->persist($itemEntity);
                    $em->flush($itemEntity);
                    $item['maxScore'] = $exercise->get_max_score();

The problem is with $exercise->get_max_score(), which is not returning the right value. In my case it's returning 3 for a test that should be returning 5.

peterdanielmyers commented 3 years ago

Ok, I've chased down what is causing this bug. It is a consequence of another bug.

In exercise.class.php https://github.com/chamilo/chamilo-lms/blob/e60243ae5713557ecfbf83c5542af84af1f3cdba/public/main/exercise/exercise.class.php#L20

In the class function:

public function get_max_score()

(currently on ln 7918)

The following two class properties both have the incorrect value of 0:

$this->random and $this->randomByCat

As a result the if statement in this function always resolves to "else", and therefore the function returns an incorrect number for the max score.

peterdanielmyers commented 3 years ago

In contrast, the test screen correctly identifies the number of questions available in a test, e.g. from the test I've been using exploring this:

image

Can someone point me to the code for that? As if the logic there was moved to the Exercise class get_max_score function, then this could be resolved pretty quickly.

peterdanielmyers commented 3 years ago

Solved it. It's a bit of a hack, as I haven't addressed the problem with the wrong values in the database.

We can use the code here https://github.com/chamilo/chamilo-lms/blob/609d23b1104f6fc5c631089b2d1c9cfc76327269/public/main/exercise/admin.php#L348-L356

Modified like so, literally just to replace the variable names:

            $out_max_score = array_reduce(
            $this->selectQuestionList(true, true),
            function ($acc, $questionId) {
                $objQuestionTmp = Question::read($questionId);

                return $acc + $objQuestionTmp->selectWeighting();
            },
            0
            );

The modified code can replace all this:

https://github.com/chamilo/chamilo-lms/blob/e60243ae5713557ecfbf83c5542af84af1f3cdba/public/main/exercise/exercise.class.php#L7921-L7969

Is there any chance this could be implemented in the next update? I'd really like to have a working learning path as soon as possible!

ywarnier commented 1 year ago

This should have been included in 1.11.18 and should be present in 2.0.

ywarnier commented 7 months ago

I'm testing this on Chamilo 2's dev code (not yet alpha, but that code base should be similar to 1.11).

This issue can thus be closed in regards to 2.0. Sorry if this is not in 1.11 but from what I can see, it should be.