RE: [Mantisbt-dev] Another day working on the custom fields

[<Chr...@he...>]

From: ju...@be... [mailto:ju...@be...]
>
> Christian, I applied this stuff.
>

Great!
 
> I reviewed the modified files but haven't had a chance to look very 
> carefully at the added files yet, so expect more on those in the next 
> day or to.  

OK. 

> At first glance, I think I'd like to organize the UI for 
> this a little but, like I said, I haven't really looked at it in much 
> detail yet.

I was also thinking if we want to increase the current project managing 
functionality (as the code is doing it at the moment; this is 
compareable to the way we manage cetegories) or if we want to generate 
a new managing system (comparable to the way we are managing projects). 
Or probably even a totally new system.

I can see advantages in both ways. What are the other developers 
thinking?

> In applying the patch I made the following changes:
> 
> [...]

Sounds very sensible

> Some thoughts or questions that came up as I was reviewing:
> 
> - should we use 'name' instead of 'caption'?  I find caption 
> kind of weird

I also find caption strange. But this was in the specs...

> - custom_field_create() and custom_field_update() could get passed a 
> CustomField object (look at how the user_pref_api does it)

The create function doesn't need it. The code is much simpler by 
creating a default field and then updating it's definiton

But passing an object or an array is both ok with me. I can't see a big 
improvement by switching to objects but I haven't used objects much, so 
I can't be a good judge.
Perhaps you could change that code? 

> - custom_field_bind() seems to be almost exactly the same as 
> custom_field_add() that was already there...

Oh, yes, you are right. Implementing the pages I totally forgot about 
that function.

Actually I think "bind" is a better word that "add" in this case. The 
custom_field_bind()function already uses the extra table to link fields 
to projects. The custom_field_add() function doesn't do that.
So we can get rid of custom_field_add().

> - custom_field_update(): that's some really really repetitive 
> code!  At 
> very least you could do that by looping over an array of field names. 
> But I would suggest we do like the user_pref_api and take an 
> object.  So 
> the patter is that you request the field, change what you 
> want, and then 
> pass it back into the update function.  That way the update 
> function can 
> update everything but only the values you change will actually change.

As I said above: I'm not very familiar with PHP objects. So it's 
probably best if you could change that.

> - I still don't like the dual functionality of 
> custom_field_get_ids(). 
> There are two distinct queries being executed that only share 
> "db_query($query)".  Sharing that line of code isn't enough reason to 
> put them in one function.  I'd like to add, maybe, 
> custom_field_get_bound_ids( $p_project_id ) or something and make the 
> existing one not take any parameters

we can do that as well

> - In manage_proj_edit_page.php:
>     + you call helper_alternate_class() with $i but there is 
> no $i defined

ah, that happens when you copy an existing page and modify it till it 
fits...

>     + you keep getting the list of field ids, you could get 
> it once and 
> use it multiple times (make sure the debugging stuff that 
> prints out the 
> SQL queries is turned on while you're developing)

thanks for the hint

>     + I'm not sure you should be able to add a new field 
> directly to a 
> project.  It would be nice to separate the interface between defining 
> fields and adding them to projects.  You might want only admins to be 
> able to define fields, but any developer to be able to add them to a 
> project, for example.  So you can add a field from a list to 
> a project, 
> or go to a list of fields that are available that allows you to edit 
> their definitions (that could be scary... making sure the values all 
> still meet a new regex, for example) or define a new one.  
> Actually, I 
> would add this as another menu item on the manage page I think: '[ 
> Manage Users ][ Manage Projects ][ Manage Custom Fields ]...' and let 
> you add fields in the manage projects section

Exactly. As above I'm interested in the opinion of other developers as 
well as I can see benefits in both ways.

> - Why does manage_proj_custom_field_edit_page.php need to take 
> f_project_id?  It isn't used and the field definitions are 
> independent 
> of the project

That's just historical. You are right and we should delete it.

CU,
Christian