[Freeipa-devel] [PATCH] HBAC Details Page

[Adam Young]

On 11/03/2010 08:30 AM, Endi Sukma Dewata wrote:
> On 11/1/2010 12:35 PM, Adam Young wrote:
>> NACK, based on the templating issues we discussed on the phone.
>>
>> TO lay out the issues for other people reading: we previously had a
>> framework like what Endi is proposing here. We found that importing HTML
>> fragments didn't provide much benefit, and lead to duplicated code. When
>> Pavel did a major rework of the code in August, we removed the
>> templating mechanism.
>>
>> We are going to add it back in as part of this patch, but with a note
>> that it is purely for Rapid prototyping purposes. For HBAC, Endi is
>> going to make the Table code into a component that works without
>> templating. Instead, weare going to generate the table code using
>> Javascript, the same way that we do in the search code. This is the
>> start of the work specified in
>>
>> https://fedorahosted.org/freeipa/ticket/419
>>
>> When we are done, we hope to havea reusable component that supports the
>> search, associtions, record level attributes (like phone number) and the
>> HBAC use cases. We'll produce a design document as we get better 
>> clarity.
>
> Please take a look at the new patch. Thanks!
>
> https://fedorahosted.org/reviewboard/r/99/
>
> The UI framework has been extended to include a collection of widgets:
>   - ipa_widget: base class
>   - ipa_text_widget: text field
>   - ipa_radio_widget: radio button
>   - ipa_textarea_widget: textarea
>   - ipa_button_widget: button
>   - ipa_column_widget: column for table
>   - ipa_table_widget: table
>
> These widgets can be used to create input controls. They can also be
> extended to create custom controls.
>
> The framework has also been enhanced to support themes. Themes can be
> used to change the look of the application without changing the code.
> This would be useful to customize IPA deployments. Initially this is
> only available in details section.
>
> A theme is a collection of HTML templates. Each template is a complete
> and valid HTML file which can be viewed using a browser. The template
> will be loaded and initialized by the code, then filled with the data
> from the server. The themes are located in install/static/themes folder.
>
> By default, if no templates are used, the fields in the details page
> are rendered vertically using dd/dt/dd tags. For pages that require
> different layout, a custom UI needs to be developed. There are two ways
> to do that:
>   - write a custom widget to generate the UI dynamically
>   - create an HTML template and write the initialization code
>
> For components that are quite complex or used frequently, it's might
> be better to use the first method. For simple pages that are used only
> in one location or need to support customization, the second method
> might be preferable. Other benefits of templates:
>   - cleaner code and UI separation
>   - more flexibility in customization
>   - new pages can be developed quickly and require less coding
>   - multiple templates can be used with the same initialization code
>   - easier to maintain
>
> The HBAC details page has been implemented using both methods. By
> default it will use custom widgets to generate the page. To use the
> theme, add the following parameter to the URL, then reload the page:
>
> &theme=<theme name>
>
> Currently the only available theme is 'default' which produces the
> same layout as the custom widgets.
>
> The HBAC details page is usable, but it still needs additional work.
> The access time is not working yet. There is no undo button, hint,
> or validation yet.
>
> The table in the association facet has also been changed to use
> ipa_association_widget which is derived from ipa_table_widget.
>
> The Makefile has been updated to include the themes. The unit tests
> also have been updated.
>

A few questions (and tweaks).  Note that I have just given the code a 
read through, not applied the patch yet.


Are you sure we want to implement our own Theme code? I'd rather try to 
keep theme stuff as part of JQUery.UI.  At a mionimum, we risk name 
clash and confusion over the term 'theme'.

add.js line 34:  Do we really need accesor like this?  There is nothing 
wrong with doing modifying the member directly.  I see the code at line 
62 that delegates it down the tree...I think there is a more 
javascript-y way to do this.  Look up Javascript accessors.


If you are going to change a function header like on associate line 133, 
go ahead and remove the camel_casing as well. (manyObjPKey) as you seem 
to be doing variable cleanup elsewhere.

Line 297, executor takes 7 params, that are all member variables of 
"that".  Since that.execute is invoked as a method, you can remove these 
parameters and instead, internal to executor, refer to them via this.<param>


Typo line 344: that.member_attrribute