Re: [Freeipa-devel] [PATCH] HBAC details page enhancement
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
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
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
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
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
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
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
