Menu
▾
▴
[Mantisbt-dev] Custom Fields Review
|
From: Victor B. <vb...@op...> - 2002-10-15 09:00:32
|
Hi Christian,
I had a look at the first version of the code that you sent. Following are
my comments. I attached the version I reviewed (since I am referring to
line numbers).
additional_tables.sql
=====================
17: CREATE TABLE mantis_custom_fields (
Although this is not in the specs that I sent to the news group, but I
think
it might be a good idea to add an "sequence" (order is reserved!) field
which would specify the order by which the fields should be listed on
the
web pages.
bug_api.php
===========
8: function bug_project_id( $p_bug_id ) {
Use bug_get_field() to use caching + no need to add extra functions that
are
not needed.
custom_fields_API.php
=====================
16: # returns an array of ids of custom fields related to the specified
project.
Replace spaces with tabs (in all files). Tab spacing 4.
17: function custom_field_def_get_ids ( $p_project_id ) {
Should this handle also fields common to all projects? Will we have for
project 5, the custom fields for project 5 + those that are associated
with
id 0 (common)?
I am thinking of dropping the requirement for general fields, or making
it a
future requirement. However, when moving a bug from one project to
another
we should maintain the custom values that have same def + same caption.
22: " WHERE project_id='$c_project_id'";
Consider adding ORDER BY sequence ASC. Hence, the UI code can use this
API
to get the IDs and display their corresponding fields/values in the
right
sequence.
31: function custom_field_exists( $p_bug_id, $p_field_id ) {
Consider adding custom_ensure_field_exists() this should trigger an
error if
the field does not exist. It should be use where a field that does not
exist is considered to be an error rather than an possible normal case.
31: function custom_field_exists( $p_bug_id, $p_field_id ) {
Use the $p_bug_id here is a bit confusing. It implies that you are
check
whether the custom field exists for this bug rather than for its
project. I
would replace this with $p_project_id. For convenience, this function
should be programmed in a way to handle the return value of
bug_project_id().
If we happen to support general fields, then we should support the case
where the project id is equal to 0.
64: if ( !custom_field_exists( $p_bug_id, $p_field_id ) )
Move this call to the top of the function (i.e. before cleaning
variables).
Consider using custom_ensure_field_exists().
68: config_get( 'mantis_custom_fields_strings' ) .
I think this should be read from the mantis_custom_fields rather than
the
strings. I also think you should read the default in this query rather
than
executing another query if the value is not found.
74: $t_project_id = db_unprepare_int( $row['project_id'] );
As jfitzel pointed out, I don't think we need to un-prepare values read
from the db.
90: $query = "SELECT default FROM " .
Returning the default for custom fields that are not found is an issue
in my
opinion. If you change the default from X to Y, then for all bugs with
this
field not explicity sit it will return Y, but for others were the
default
was explicitly set it will return X. This will be a problem since when
you
update a bug the update code might write the value explictly. Even if
it
does not, then you updated with X and you are getting a Y!.
This needs further discussion.
108: if ( !custom_field_exists( $p_bug_id, $p_field_id ) )
Move to top of function and consider using custom_ensure_field_exists().
119: $t_project_id = db_unprepare_int( $row['project_id'] );
Not needed.
128: if ( !ereg( $t_regexp, $c_value ) )
Which one would we want to use ereg or preg? An error message should be
triggered if this check fails.
137: if( !access_level_check_greater_or_equal( $t_access_level_rw,
$t_project_id ) )
This should be the first check. There is no point validating the field
if
you don't have the access right to overwrite it.
141: $query = "SELECT COUNT(*) FROM " .
SELECT 1 should be faster than SELECT COUNT(*). Also use LIMIT 1 to
indicate that there can only be one of these.
159: db_querry( $query );
This line can be factored outside the if{}else{}.
207: $c_access_level_r = 0;
Add a define to the access level called ANYBODY and set it to 0. This
is to
be done in constant_inc.php.
230: $query = "INSERT INTO " .
You will need to add the sequence field here.
241: if ( false == db_querry( $query ) )
db_querry->db_query. BTW, db_query will exist in case of false and
hence
you do not need to handle this case.
248: # returns an associative array with all fields.
with all field attributes.
255: config_get( 'mantis_custom_fieldss' ) .
remove extra s.
260: if( false == $result )
not needed.
269: function custom_field_def_update ( $p_project_id, $p_field_id,
$p_def_array ) {
In the code below, you seem to support incremental update. I don't
think we
need to support this. We also need to have some code to
default/validate
this structure which should be used in both the defining of a new field
and
the updating of a field definition.
308: " AND project_id='$c_project_id'";
Add LIMIT 1.
310: if ( false == db_querry( $query ) )
no need to handle the return value.
320: function custom_field_def_delete ( $p_project_id, $p_field_id ) {
This should only require the field id as a parameter since this is
unique by
itself. The project id is not needed. See primary key in the table.
This
might apply to other function. I am aware that this is an issue with
the
APIs in the specs.
325: $query = "SELECT COUNT(*) FROM " .
I don't think this query is necessary, just go ahead and run the two
delete
queries anyway.
366: $c_project_id = db_prepare_int( $p_project_id );
No need to prepare project id.
366: $c_project_id = db_prepare_int( $p_project_id );
No need to prepare the parameter, if you are only sending it to other
APIs.
370: for( $index=0; $index < count( $t_field_ids ); $index++ )
Use foreach syntax instead.
370: for( $index=0; $index < count( $t_field_ids ); $index++ )
Use foreach syntax instead.
Regards,
Victor.
|
