New personality-test quiz type #3092
Conversation
Applied fixes from FlintCI
|
Lint errors were found. A patch is also available. Please see the report: https://flintci.io/repositories/1013/analyses/30594 This comment was posted by FlintCI. It can be disabled in the repository settings. |
|
Lint errors were found. A patch is also available. Please see the report: https://flintci.io/repositories/1013/analyses/30629 This comment was posted by FlintCI. It can be disabled in the repository settings. |
|
Issues
======
+ Solved 9
- Added 16
Complexity increasing per file
==============================
- main/inc/lib/exercise_show_functions.lib.php 1
- main/exercise/ptest_category_ranking.class.php 15
- main/exercise/ptest_agree_disagree.class.php 15
- main/exercise/ptest_category.class.php 5
- main/exercise/ptest_agree_scale.class.php 15
- main/exercise/ptests_category.php 5
- main/exercise/ptest_agree_reorder.class.php 15
- main/exercise/answer.class.php 1
See the complete overview on Codacy |
| @@ -0,0 +1,296 @@ | |||
| <?php | |||
There was a problem hiding this comment.
| public function getCategories($exerciseId, $courseId, $sessionId = 0) | ||
| { | ||
| $table = Database::get_course_table(TABLE_QUIZ_CATEGORY_PTEST); | ||
| $itemProperty = Database::get_course_table(TABLE_ITEM_PROPERTY); |
There was a problem hiding this comment.
| @@ -0,0 +1,157 @@ | |||
| <?php | |||
There was a problem hiding this comment.
| @@ -0,0 +1,476 @@ | |||
| <?php | |||
There was a problem hiding this comment.
| @@ -0,0 +1,298 @@ | |||
| <?php | |||
There was a problem hiding this comment.
| @@ -0,0 +1,271 @@ | |||
| <?php | |||
There was a problem hiding this comment.
| @@ -0,0 +1,255 @@ | |||
| <?php | |||
There was a problem hiding this comment.
| @@ -0,0 +1,317 @@ | |||
| <?php | |||
There was a problem hiding this comment.
| @@ -0,0 +1,115 @@ | |||
| <?php | |||
There was a problem hiding this comment.
| @@ -0,0 +1,530 @@ | |||
| <?php | |||
There was a problem hiding this comment.
| @@ -0,0 +1,296 @@ | |||
| <?php | |||
There was a problem hiding this comment.
| @@ -0,0 +1,223 @@ | |||
| <?php | |||
There was a problem hiding this comment.
| @@ -0,0 +1,348 @@ | |||
| <?php | |||
There was a problem hiding this comment.
| $_POST[$key] = stripslashes($val); | ||
| } elseif (is_array($val)) { | ||
| foreach ($val as $key2 => $val2) { | ||
| $_POST[$key][$key2] = stripslashes($val2); |
There was a problem hiding this comment.
Se que estamos haciendo algo así en global.inc.php, pero es algo que siempre hemos querido quitar. Escribir en $_POST no es una buena práctica...
| Session::write('objExercise', $objExercise); | ||
| Session::write('objQuestion', $objQuestion); | ||
| Session::write('objAnswer', $objAnswer); | ||
| Display::display_footer(); |
There was a problem hiding this comment.
No estoy seguro de qué cantidad de código aquí es realmente necesaria. Veo mucha repetición con exercise_admin.php (si no me equivoco) pero no me queda claro cual es la parte añadida para ptest y si no se podría poner en el mismo archivo exercise_admin.php...
There was a problem hiding this comment.
Wow, primero felicitarte Jose Angel (@nosolored) porque hay mucho trabajo aquí y solo genera unos cuantos pequeños reportes en cuanto al estilo del código. Por esto, gracias.
Ahora, lamentablemente, como es mucho código de una vez, tengo muchas observaciones. Algunas las he puesto a modo de review en tu PR, otras no puedo y las voy retomando aquí:
- A gran escala, creo que hubiera sido una buena idea consultar conmigo antes de lanzar el desarrollo, porque ya tenemos una funcionalidad similar abandonada a nivel de encuestas, que es el lugar adecuado para un sistema de evaluación que no genere notas... ahora tenemos dos sistemas en paralelo. Me da pena porque veo que el vuestro se ve muy bien y se que el otro no es activable en el estado actual. Sin embargo, la introducción de este tipo de ejercicio introduce otros temas más complejos...
- No he visto ningún código relacionado con la herramienta de evaluaciones (Gradebook). En esta herramienta, se puede seleccionar cualquier ejercicio de la herramienta de ejercicios. Sin embargo, como aquí no generamos una nota, esta inclusión debería ser prohibida. Hay que modificar la función que obtiene los ejercicios del curso.
- gracias por renombrar variables (si lo hiciste tu) en camelCase. Lamentablemente, en el contexto de un gran PR como este, es mejor no hacerlo (y mandarlo en un PR a parte), porque reduce la legibilidad de tu PR. Distrae con muchas líneas que no tienen que ver, y uno tiene que preguntarse a cada momento si algo más cambió en esta sección que el nombre de la variable. En sí, para hacerlo un poco más legible, tuve que usar el parámetro "?w=1" en Github para que me escondiera los cambios que solo son de unos espacios en blanco. Los cambios de espacios en archivos (como los de main/lp/ o main/session/) no relacionados con el objeto de este PR deben ser eliminados/restaurados, para que se pueda hacer merge de manera limpia.
- Existen varios cambios misteriosos de creación o borrado de funciones, que parecen ser por error. He apuntado algunos.
- no he probado el código todavía, pero entiendo que es necesario tener otro tipo de tipo de ejercicio (c_quiz.ptype) porque no queremos que los distintos tipos de "ptest" se presenten todos en la lista de tipos de feedback en la creación del ejercicio. Estoy en lo correcto? Caso contratio, sería mucho mejor mezclar estos tipos y evitar la creación de un campo adicional en la tabla c_quiz (esto representa trabajo adicional de mantenimiento).
- encuentro pocas descripciones de funciones en la documentación PHPDoc al inicio de la función.
- me parece (es difícil verlo con claridad sin conocer las funciones de memoria) que se ha copiado mucho código (duplicación de funciones) con el objetivo de evitar mezclar el nuevo código con código anterior. Sin embargo, de esta manera se genera repetición de código, que es considerado malo (porque aumenta los lugares donde hay que modificar algo cuando haya un cambio a futuro) y nos impactará negativamente en https://scrutinizer-ci.com/g/chamilo/chamilo-lms/statistics/ A veces no hay opción, pero por lo general se debería intentar mejor llamar a una función adicional dentro de una función existente, a copiar toda la función solo para añadir 20 líneas en su copia.
- quedaron unas observaciones automatizadas de Codacy sin atender desde la fecha de creación del PR...
- en el caso de nuevos iconos PNG (es el caso aquí), les pedimos que también provean una versión SVG para cada uno. En la mayoría de los casos, se puede generar fácilmente en base a un icono SVG ya existente (están en main/img/icons/svg). Esto nos permite, a partir de ahora, asegurarnos que podamos generar nuevos iconos de cualquier tamaño para evoluciones futuras.
- Sería mejor intentar encontrar otro mecanismo que la adición de parámetros a una función tan usada como Question::read(). Al modificar el esquema (aunque sea con variables opcionales), se generan cambios en todos los procesos de validación del código. Una vez más: a veces no hay opción, pero es necesario asegurarse de esto primero.
- Como lo noté a bajo (pero no en todos los lugares), la adición de c_quiz.ptype como se presenta en este PR genera una dependencia obligatoria sobre la existencia de este campo en la tabla c_quiz (y del campo ptest_category en la tabla c_quiz_question). Al correr "composer install", el esquema de entidades cambiará, haciendo que Doctrine espere encontrar un campo "ptype" en esta tabla. Si es que no se ha activado esta funcionalidad (y por consecuente no se han creado los campos y la tabla relacionados), Doctrine protestará de que no puede encontrar la información "esquematizada", y generará un error fatal. Una forma de evitar esto para otros desarrollos que hicimos, es quitar el @ adelante del @Orm dentro de los comentarios del nuevo campo en la entidad, y pedir al usuario que active la funcionalidad (con los queries SQL) que vaya agregar también un @ adelante de ORM en las 2 clases correspondientes.
Sin embargo, esto no bastará aquí porque también has metido ptype dentro de los constructores de answer.class.php etc. Una solución queda por encontrar ahí.
| use ChamiloSession as Session; | ||
|
|
||
| /** | ||
| * Class UniqueAnswer. |
There was a problem hiding this comment.
Modificar el nombre de la clase en la documentación también.
| use ChamiloSession as Session; | ||
|
|
||
| /** | ||
| * Class UniqueAnswer. |
There was a problem hiding this comment.
Modificar el nombre de la clase en la documentación también.
| use ChamiloSession as Session; | ||
|
|
||
| /** | ||
| * Class UniqueAnswer. |
There was a problem hiding this comment.
Modificar el nombre de la clase en la documentación también.
| @@ -521,8 +521,6 @@ function confirm_your_choice() { | |||
| <?php | |||
|
|
|||
| /** | |||
| * @param array $formValues | |||
| * | |||
There was a problem hiding this comment.
Limpieza? Mejor aumentar los parámetros que faltan, que reducir los que hay en la documentación...
| @@ -57,6 +57,7 @@ | |||
| $htmlHeadXtra[] = '<link rel="stylesheet" href="'.api_get_path(WEB_LIBRARY_JS_PATH).'hotspot/css/hotspot.css">'; | |||
| $htmlHeadXtra[] = '<script src="'.api_get_path(WEB_LIBRARY_JS_PATH).'hotspot/js/hotspot.js"></script>'; | |||
| $htmlHeadXtra[] = '<script src="'.api_get_path(WEB_LIBRARY_JS_PATH).'annotation/js/annotation.js"></script>'; | |||
| $htmlHeadXtra[] = api_get_js('chartjs/Chart.min.js'); | |||
There was a problem hiding this comment.
Sería bueno condicionar esto a la activación de ptest (api_get_configuration_value('...'))
| * | ||
| * @return bool True if the score should be used as progress, false otherwise | ||
| */ | ||
| public function getUseScoreAsProgress() |
There was a problem hiding this comment.
No entiendo porqué se movieron estos dos métodos (no estoy seguro de si hay algún cambio o no)
There was a problem hiding this comment.
Respuesta luego de revisar: no hay cambios.
| @@ -293,7 +293,8 @@ function show_cols(grid, added_cols) { | |||
|
|
|||
| // Sortable rows | |||
| grid.jqGrid('sortableRows', options); | |||
| <?php } ?> | |||
| <?php | |||
| } ?> | |||
There was a problem hiding this comment.
Este cambio tampoco tiene sentido en el contexto del PR
| @@ -10,7 +10,7 @@ | |||
| use Fhaculty\Graph\Vertex; | |||
|
|
|||
| /** | |||
| * Class SequenceResourceRepository | |||
| * Class SequenceResourceRepository. | |||
There was a problem hiding this comment.
Debería quedar sin punto final. Es el nombre de la clase, no es una frase.
| * | ||
| * @return array | ||
| */ | ||
| protected function findSessionFromVerticesEdges(Vertices $verticesEdges) |
There was a problem hiding this comment.
Tampoco entiendo la aparición de este método. Supongo que es porque se mandó el PR anteriormente y que, desde entonces, hemos quitado este método en Chamilo, pero no estoy seguro...
| /** | ||
| * @var string | ||
| * | ||
| * @ORM\Column(name="ptest_category", type="text", nullable=true) |
There was a problem hiding this comment.
El problema de esto es que va a hacer fallar (Fatal error) el código para los portales donde el ptest no está activado (mejor dicho donde el campo ptest_category no existe).
|
@ywarnier por todo lo que comentas, ¿piensas que es mejor no integrarlo en Chamilo por ahora? |
|
@nosolored Sí, creo que es mejor dejarlo como un PR abierto por ahora. Lo veo super complicado incluirlo en 1.11. |
|
De acuerdo entonces, lo dejamos así por ahora |
|
Solo para hacer un update sobre este PR : Se podría integrar en Chamilo 2.0 a la condición que el código esté listo y pasado a encuestas para fines de abril. Conociendo la situación actual, no creo que sea factible, pero guardo el PR abierto por si a caso nosotros tenemos tiempo para hacer la conversión. No están soportadas las encuestas en las lecciones todavía, pero se puede añadir su soporte tomando en cuenta que se consideraría un paso "completado" cuando haya sido respondida en full. |
| // description LONGTEXT, | ||
| // session_id INT NOT NULL, | ||
| // PRIMARY KEY(id)) DEFAULT CHARACTER SET utf8 COLLATE utf8_unicode_ci ENGINE = InnoDB; | ||
| // ALTER TABLE c_quiz ADD COLUMN pt_type INT DEFAULT 0 NOT NULL; |
There was a problem hiding this comment.
Me parece que c_quiz.pt_type podría simplemente usar c_quiz.type, ya que no se van a usar al mismo tiempo pt_type y type.
There was a problem hiding this comment.
También hay posibilidad de que la tabla c_quiz_category_ptest pueda ser remplazada por un uso de c_quiz_question_category.
|
In the current state, we will not be able to merge this feature into C2.0 (too many changes to apply to match the new structure of Chamilo). |
Nuevo tipo de ejercicios con 4 tipo de cuestiones diferentes. A diferencia de los ejercicios clásicos de Chamilo, no se puntúan, están más orientados a lo que podría ser ejercicios de personalidad, en los cuales al terminar de responder el ejercicio te muestra un gráfico de barras y de "radar" con las preferencias elegidas.
