Implement Process error table #136
Conversation
justusdieckmann
commented
Dec 7, 2021
justusdieckmann
commented
justusdieckmann
force-pushed
the
feature/admin-error-page
branch
from
667488d to
280fd40
Compare
justusdieckmann
force-pushed
the
feature/admin-error-page
branch
from
280fd40 to
8054270
Compare
justusdieckmann
force-pushed
the
feature/admin-error-page
branch
from
14e8d96 to
c8f2313
Compare
| echo $this->get_dynamic_table_html_start(); | ||
|
|
||
| // Render button to allow user to reset table preferences. | ||
| echo $this->render_reset_button(); |
There was a problem hiding this comment.
We have already asked Aaron about this. Do we really need these buttons for an empty table? We have seen that in multiple plugins but why is it necessary?
There was a problem hiding this comment.
I don't really know, I just copied the print_nothing_to_display() function and edited the string
There was a problem hiding this comment.
Wird schon seine Gründe haben
db/install.xml
Outdated
| <FIELD NAME="errormessage" TYPE="text" NOTNULL="true" SEQUENCE="false" COMMENT="Message of the error"/> | ||
| <FIELD NAME="errortrace" TYPE="text" NOTNULL="true" SEQUENCE="false"/> | ||
| <FIELD NAME="errorhash" TYPE="char" LENGTH="255" NOTNULL="true" SEQUENCE="false" COMMENT="Where the error occured in the form 'path/to/filename.php:line'"/> | ||
| <FIELD NAME="errortime" TYPE="int" LENGTH="11" NOTNULL="true" SEQUENCE="false" COMMENT="unix timestamp - time the error occured"/> |
There was a problem hiding this comment.
It would be more intuitive to use the Moodle standard naming conventions (timecreated instead of errortime).
There was a problem hiding this comment.
My reasoning was, that timecreated might be ambigous in this context, since this table copies all fields from the process table, it could mean time the process was created. To make things clear I prefixed all new fields with error. Do you think that justifies the diversion?
There was a problem hiding this comment.
We see your point however errortime might also be ambiguous, how about errortimecreated?
db/upgrade.php
Outdated
| $table->add_field('errormessage', XMLDB_TYPE_TEXT, null, null, XMLDB_NOTNULL, null, null); | ||
| $table->add_field('errortrace', XMLDB_TYPE_TEXT, null, null, XMLDB_NOTNULL, null, null); | ||
| $table->add_field('errorhash', XMLDB_TYPE_CHAR, '255', null, XMLDB_NOTNULL, null, null); | ||
| $table->add_field('errortime', XMLDB_TYPE_INTEGER, '11', null, XMLDB_NOTNULL, null, null); |
justusdieckmann
force-pushed
the
feature/admin-error-page
branch
from
7b97743 to
39f8597
Compare
|
Generally for best practices, we should write more tests. But it all looks good, thanks! |
