Re: [Freeipa-devel] [PATCH] HBAC details page enhancement

2010-11-15 Thread Adam Young

On 11/13/2010 11:14 PM, Endi Sukma Dewata wrote:

Hi,

Please review the attached patch. Thanks!

https://fedorahosted.org/reviewboard/r/107/

The HBAC details page has been enhanced to support Undo and Reset 
operations. The functionality is implemented in the base widget class 
so the behavior will be more consistent across widgets. A span tag 
now used to define the field boundary in the HTML doc. The tag 
contains the visual representation of the field which include the 
input tag and optionally the undo link.


The Update method on HBAC details page has been modified so that it 
executes several operations using a batch command. The operations 
being executed depends on the changes made to the fields. These 
operations may include:

 - removing access time if access time is changed to any time


This didn't seem to work.  The rest of it was fine, but I still see the 
acces time I added



 - removing memberships if member category is changed to all
 - modifying rule attributes if description or rule type is changed
 - enabling/disabling the rule if rule status is changed

The behavior of the Add  Remove buttons also has been changed such 
that it adjust the category attribute properly in addition to adding the
memberships using batch command. For example, if category is initially 
set to all, adding a new member will also change the category to empty.


The ipa_command have been modified to store the on_success and 
on_error handlers as properties. When the command is executed as a 
part of batch operation, the result of each command will be passed to 
the appropriate handler.


The unit tests and test data have been updated as well.


___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel


___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Re: [Freeipa-devel] [PATCH] HBAC details page enhancement

2010-11-15 Thread Adam Young

On 11/15/2010 11:00 AM, Adam Young wrote:

On 11/15/2010 10:58 AM, Adam Young wrote:

On 11/13/2010 11:14 PM, Endi Sukma Dewata wrote:

Hi,

Please review the attached patch. Thanks!

https://fedorahosted.org/reviewboard/r/107/

The HBAC details page has been enhanced to support Undo and Reset 
operations. The functionality is implemented in the base widget 
class so the behavior will be more consistent across widgets. A 
span tag now used to define the field boundary in the HTML doc. 
The tag contains the visual representation of the field which 
include the input tag and optionally the undo link.


The Update method on HBAC details page has been modified so that it 
executes several operations using a batch command. The operations 
being executed depends on the changes made to the fields. These 
operations may include:

 - removing access time if access time is changed to any time


This didn't seem to work.  The rest of it was fine, but I still see 
the acces time I added



 - removing memberships if member category is changed to all
 - modifying rule attributes if description or rule type is changed
 - enabling/disabling the rule if rule status is changed

The behavior of the Add  Remove buttons also has been changed such 
that it adjust the category attribute properly in addition to adding 
the
memberships using batch command. For example, if category is 
initially set to all, adding a new member will also change the 
category to empty.


The ipa_command have been modified to store the on_success and 
on_error handlers as properties. When the command is executed as a 
part of batch operation, the result of each command will be passed 
to the appropriate handler.


The unit tests and test data have been updated as well.


___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel



___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

BTW, I rebased and merged on top of my one line fix.  Here's the updated.


___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

ACK
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Re: [Freeipa-devel] [PATCH] HBAC details page enhancement

2010-11-15 Thread Endi Sukma Dewata

On 11/15/2010 11:47 AM, Adam Young wrote:

ACK


Thanks. Pushed a new rebase to master.

--
Endi S. Dewata

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel


Re: [Freeipa-devel] [PATCH] HBAC Details Page

2010-11-03 Thread 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


___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel


Re: [Freeipa-devel] [PATCH] HBAC Details Page

2010-11-03 Thread Adam Young

On 11/03/2010 10:48 AM, Adam Young wrote:

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


___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel



Also: remove the buttons for features that we are not going to implement 
this time around


from the top of the page: 

Re: [Freeipa-devel] [PATCH] HBAC Details Page

2010-11-03 Thread Endi Sukma Dewata

On 11/3/2010 10:09 AM, Adam Young wrote:

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'.


I think they will complement each other. The jQuery UI theme is limited 
to CSS. The IPA theme I'm creating is for the layout. Although CSS can 
do layout too, it is limited to the elements and classes that are 
already defined in the HTML page. If you need to change the elements in 
the HTML page you'd have to change the JavaScript code because it's 
generated dynamically. For example, it's not possible to change this layout:


  First Name: Adam
  Last Name:  Young

into this layout

  Last Name: Young   First Name: Adam

using CSS alone unless you specify different ID's or assign different 
CSS classes for each HTML element. And changing the JavaScript code to 
support this specific layout would be either too difficult or not very 
useful for anything else.


With the IPA theme you could create 2 different templates:

dl
   dtFirst Name:/dt
   ddspan name=firstName//dd
   dtLast Name:/dt
   ddspan name=lastName//dd
/dl

or

table
tr
   tdLast Name:/td
   tdspan name=lastName//td
   tdFirst Name:/td
   tdspan name=firstName//td
/tr
/table

and not change a single code. The jQuery UI theme can still be applied 
on top of this.



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.


You mean like this? http://offthelip.org/?p=101
Yes, we can do that. Regardless, the accessor is necessary because a 
widget may contain a set of other widgets and we want to let the widget 
figure out how to pass the value to the other widgets. Ideally I prefer 
if we can get rid of the entity_name from widgets, but we can do that 
another time.



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.


OK, I can fix that too. I had to draw the line somewhere, otherwise the 
patch will be too big (it's already bigger than I wanted).



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


Not sure that would be a good idea. that in this code refers to the 
dialog box. The executor is one of the serial/bulk associator. 
Associator is not a dialog box, so referring to the parameters using 
this might work but will be confusing. This code is actually calling the 
associator's constructor and execute it too. I was planning to create a 
base class for the associators, but that's for next time.



Typo line 344: that.member_attrribute


OK, I'll fix it. That was from a copy  paste.


Also: remove the buttons for features that we are not going to implement
this time around

from the top of the page: Troubleshoot, Cull Disabled Rules, And the
TEst Rule link under quick links
You can leave Login SVC and Login Svc Groups , those are coming next,
correct?


Let me remove all of them for now and add it back as I implement them. 
That way we can cut a release anytime without having a broken button.



Add rule has a rule type field, but no guidance what to fill it in with.
I suspect this should be a select. Without knowing what to put in here,
you can't add a rule. At a minimum, lets put in text 'allow or deny'


OK, I'll add that text. I was planning to do that in a later patch 
because we have the same issue with the service name.



Note that this failure case doesn't fail very cleanly. There is an error
that shows up in Friebug. Ignore it for now, as I belive my patch for
handling ticket time out fixes this as a side effect.

Add access time seems to be broken. I get 'that.add is not a function'


Yes, I mentioned that in the patch description. That will be done in a 
follow up.


--
Endi S. Dewata

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel


Re: [Freeipa-devel] [PATCH] HBAC Details Page

2010-11-03 Thread Adam Young

On 11/03/2010 02:43 PM, Endi Sukma Dewata wrote:

On 11/3/2010 10:09 AM, Adam Young wrote:

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'.


I think they will complement each other. The jQuery UI theme is 
limited to CSS. The IPA theme I'm creating is for the layout. Although 
CSS can do layout too, it is limited to the elements and classes that 
are already defined in the HTML page. If you need to change the 
elements in the HTML page you'd have to change the JavaScript code 
because it's generated dynamically. For example, it's not possible to 
change this layout:


  First Name: Adam
  Last Name:  Young

into this layout

  Last Name: Young   First Name: Adam

using CSS alone unless you specify different ID's or assign different 
CSS classes for each HTML element. And changing the JavaScript code to 
support this specific layout would be either too difficult or not very 
useful for anything else.


With the IPA theme you could create 2 different templates:

dl
dtFirst Name:/dt
ddspan name=firstName//dd
dtLast Name:/dt
ddspan name=lastName//dd
/dl

or

table
tr
tdLast Name:/td
tdspan name=lastName//td
tdFirst Name:/td
tdspan name=firstName//td
/tr
/table

and not change a single code. The jQuery UI theme can still be applied 
on top of this.


Very cool, but suggest we change the term.  Would layout perhaps be better?




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.


You mean like this? http://offthelip.org/?p=101
Yes, we can do that. Regardless, the accessor is necessary because a 
widget may contain a set of other widgets and we want to let the 
widget figure out how to pass the value to the other widgets. Ideally 
I prefer if we can get rid of the entity_name from widgets, but we can 
do that another time.


yeah, like that.  I'd rather not use a naming convention that is 
different from what the language supports.





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.


OK, I can fix that too. I had to draw the line somewhere, otherwise 
the patch will be too big (it's already bigger than I wanted).



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


Not sure that would be a good idea. that in this code refers to the 
dialog box. The executor is one of the serial/bulk associator. 
Associator is not a dialog box, so referring to the parameters using 
this might work but will be confusing. This code is actually calling 
the associator's constructor and execute it too. I was planning to 
create a base class for the associators, but that's for next time.


Yeah.  PLus, with the Bulk plugin, we'll want to change the name of the 
bulk associator to something more correct, like single_call versus 
bulk_call, and change the serial associator to use the bulk plugin.





Typo line 344: that.member_attrribute


OK, I'll fix it. That was from a copy  paste.


Also: remove the buttons for features that we are not going to implement
this time around

from the top of the page: Troubleshoot, Cull Disabled Rules, And the
TEst Rule link under quick links
You can leave Login SVC and Login Svc Groups , those are coming next,
correct?


Let me remove all of them for now and add it back as I implement them. 
That way we can cut a release anytime 
without having a broken button.

Agreed




Add rule has a rule type field, but no guidance what to fill it in with.
I suspect this should be a select. Without knowing what to put in here,
you can't add a rule. At a minimum, lets put in text 'allow or deny'


OK, I'll add that text. I was planning to do that in a later patch 
because we have the same issue with the service name.


Yeah, lets do it now.  I had to go top the CLI to figure out what values 
to put in here to get it to work.  Add a ticket for replacing it with a 
select, too.



Note that this failure case doesn't fail very cleanly. There is an error
that shows up in Friebug. Ignore it for now, as I belive my patch for
handling ticket time out fixes this as a side effect.

Add access time seems to be broken. I get 'that.add is not a function'


Yes, I mentioned that in the patch description. That will be done in a 
follow up.




___
Freeipa-devel mailing list
Freeipa-devel@redhat.com

Re: [Freeipa-devel] [PATCH] HBAC Details Page

2010-11-01 Thread Adam Young

On 10/29/2010 07:46 PM, Endi Sukma Dewata wrote:

Hi,

Please review the attached patch. Thanks!

https://fedorahosted.org/reviewboard/r/99/

The ipa_details_section class has been enhanced to support HTML
templates. This way the layout can be changed without modifying
the code. The ipa_details_field is used to setup the fields in
the template, also used to load and save the values.

If no template is specified, it will go back to the original
behavior: the section will be rendered using the dl/dt/dd tags.

Some fields have been added to support standard HTML widgets:
 - ipa_details_text: text field
 - ipa_details_radio: radio button
 - ipa_details_textarea: textarea
 - ipa_details_button: button

The HBAC details page has been implemented using this enhancement.
It uses the templates stored in hbac-details-*.html. It also uses
HBAC-specific widgets which are defined in these classes:
 - ipa_hbac_details_table: table for member enrollment
 - ipa_hbac_details_accesstime: table for access time

The buttons for adding and removing members are still not working.
There is no hint or undo functionality yet. They will be added in
subsequent patches.

The ipa_make_button() has been converted into ipa_button class which
can be used to replace the standard HTML button. The search-container
CSS class has been renamed to entity-container and used for all facets.

The unit test and test data have been updated accordingly.


___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

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.





___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel