[Freeipa-devel] [PATCH] 0018-0030, 52 webui: add support for more certificates
Pavel Vomacka
pvomacka at redhat.com
Mon Jun 27 12:55:10 UTC 2016
On 06/23/2016 03:17 PM, Petr Vobornik wrote:
> comments inline
>
> On 06/20/2016 02:37 PM, Pavel Vomacka wrote:
>>
>> On 06/14/2016 09:41 PM, Pavel Vomacka wrote:
>>>
>>> On 05/13/2016 06:56 PM, Petr Vobornik wrote:
>>>> On 04/26/2016 04:23 PM, Pavel Vomacka wrote:
>>>>> Self-NACK for patches 0027, 28, 29, 30 - used incorrect policy. I also attach
>>>>> all patches which were not changed - it is easier to get the whole patchset.
>>>>>
>>>>> On 04/26/2016 02:02 PM, Pavel Vomacka wrote:
>>>>>> I forgot to mention that my patches requires patches from :
>>>>>> https://www.redhat.com/archives/freeipa-devel/2016-April/msg00209.html
>>>>>>
>>>>>>
>>>>>> On 04/26/2016 01:33 PM, Pavel Vomacka wrote:
>>>>>>> Hello,
>>>>>>>
>>>>>>> the attached patches add support for more certificates and ability to add and
>>>>>>> remove certificates. Fixes these two tickets:
>>>>>>> https://fedorahosted.org/freeipa/ticket/5108
>>>>>>> https://fedorahosted.org/freeipa/ticket/5381
>>>>>>>
>>>>>>> These patches add ability to view, get, download, revoke, restore and delete
>>>>>>> each certificate directly from user/host/service details page. There is also
>>>>>>> button for adding new certificates.
>>>>>>>
>>>>>>> There is one known issue, that after page save action is performed some data
>>>>>>> disappear (includes certificates). This issue has a ticket already:
>>>>>>> https://fedorahosted.org/freeipa/ticket/5776
>>>>>>>
>>>>>>> --
>>>>>>> Pavel^3 Vomacka
>>>>>>>
>>>> Great stuff, couple comments below.
>>>>
>>>> We can discuss some items in person. Not everything needs to be done.
>>>>
>>>> I didn't run it, just reading the code.
>>>>
>>>> Patch 0018:
>>>>
>>>> 1. Nit pick: When a value should be boolean, then following method won't
>>>> make sure that dropdown_menu won't be e.g. an object.
>>>> + that.dropdown_menu = spec.dropdown_menu || false;
>>>>
>>>> I would prefer:
>>>> + that.dropdown_menu = !!spec.dropdown_menu;
>>>>
>>>> Which retypes it to boolean. If default should be true (not this case) then:
>>>> that.dropdown_menu = spec.dropdown_menu !== undefined ?
>>>> !!spec.dropdown_menu : true;
>>>>
>>>> Also the interface is very specific. It says that the child widget will
>>>> have dropdown menu. What if the actions won't be in dropdown menu but,
>>>> e.g., some overlay menu.
>>>>
>>>> Imho the interface should be:
>>>>
>>>> that.custom_actions = !!spec.custom_actions;
>>>>
>>>> Than the child object would have define,e.g., :
>>>>
>>>> action_object get_custom_actions()
>>>>
>>>> Interface of action_object would be e.g.:
>>>> get_items()
>>>> set_items(items)
>>>> enable_item(name)
>>>> disable_item(name)
>>>>
>>>> Dropdown menu would have to define these methods.
>
> It calls ,render on custom actions object. But this method is not part
> of the interface:
> + custom_actions.render();
>
> IMHO it should be called internally in the widget as a result of
> set_items or disable_item, enable_item call.
Fixed.
>>>> Patch 0019:
>>>>
>>>> 1. Shouldn't disable_item or enable_item automatically rerender the items?
> Same for set and get_items. As mentioned above.
>
> The render code in disable/enable_item should be executed only if
> this.ul_node exists.
>
Fixed.
>>>> 2. The rerender, used in later patches. Imo it should do only:
>>>>
>>>> if (this.ul_node) {
>>>> construct.empty(this.ul_node);
>>>> this._render_items(this.items);
>>>> }
>>>>
>>>> Or just re-render the one item.
>>>> $( "li[data-name=" + item.name +"]", this.ul_node ).replaceWith(
>>>> this._render_item(item));
>>>>
>>>> 3. in future for loops write:
>>>> for (var i=0, l=this.items.length; i<l; i++) {
>>>> instead of
>>>> for (var i=0; i<this.items.length; i++) {
>>>>
>>>> It's defensive style when you don't know if computing length is O(1) or
>>>> O(n) or how big is n. In this instance it doesn't probably matter.
>>>>
>>>> Patch 0020:
>>>>
>>>> 1. Working widgets inherits from IPA.widget so why do you redefine
>>>> 'name', 'facet', 'entity'
>>>>
>>>> 2. why to you introduce 'base_cls' and 'other_cls' when there are
>>>> 'base_css_class' and 'css_class'?
>>>>
>>>> 3. typo in comment 'opaciti'
>>>>
>>>> 4. We can ignore IE < 9, I'm not sure if you avoid using some features
>>>> because of IE9.
>>>>
>>>> 5. _normalize_bg_color_css could be moved to util.js But I wonder if we
>>>> need it. rgba is supported in 95% of browsers and definitely in all
>>>> supported by IPA (chrome, firefox).
> Patch 0020-2: ACK
>
>>>> Patch 0021:
>>>>
>>>> 1. The original code is not great - RPC code should be agnostic to
>>>> display layer. Ideally it would not call methods like
>>>> IPA.hide_activity_icon(); and IPA.display_activity_icon();
>>>>
>>>> I would rather avoid adding more of such things there and would refactor
>>>> now, when it is easy.
>>>>
>>>> IMO the rpc class could use Evented mixin and have:
>>>> 4 local events: start, end, success, error
>>>> 2 global topics: rpc-start, rpc-end
>>>>
>>>> Local events will be new - will replace the notify_activity_start and
>>>> notify_activity_end. For convenience, spec object could accept a
>>>> handlers in similar fashion as now. The handlers would be registered to
>>>> the events.
>>>>
>>>> Global topics would replace current IPA.hide_activity_icon(); and
>>>> IPA.display_activity_icon();
>>>>
>>>> App class would subscribe to the global events and to stuff (hide|show)
>>>> accordingly. Example of such subscription:
>>>> topic.subscribe('phase-error', lang.hitch(this, this.on_phase_error));
>>>>
>>>> Then hide_activity_icon could be something like notify_global with
>>>> default to true which can be suppressed by setting it to false.
>>>>
>>>> What do you think?
> OK, if addressed later then ACK for 0021-2 which is the same as 0021
>
>>>> Patch 0022:
>>>>
>>>> 1. Leftover line:
>>>> var sn = record[i].serial_number;
> 0022-2: ACK
>
>>>> Patch 0023:
>>>>
>>>> 1. move the css code from ipa.css to layout.less?
>>>>
>>>> 2. Did you mean "display: table"?
>>>> display: cert-table;
>>>>
>>>> 3. Why are create_layout, create_row and create_cell private methods of
>>>> create_content? It would make sense to create a table dialog mixin
>>>> (similar as confirm mixin) and use the same code in all cert dialogs(if
>>>> applicable).
>>>>
>>>> 4. 'table-head' class is used in every cell. Why is it call 'head' when
>>>> it is in fact in all cells?
> 1. table mixin has prepared space for doc text but nothing is there
> 2. while at it(otherwise don't bother), remove empty line at the end of
> create_cell, create_row, create_header_cell
>
> 3. When I see a content of create_content on certificate.js:316,
> following is repeated many times, it can be changed into a reusable
> function.
>
> row = that.create_row().appendTo(table_layout);
> row.append(that
> .create_header_cell('@i18n:objects.cert.organizational_unit', ':'));
> row.append(that.create_cell(that.subject.ou, '', 'break-words'));
>
> create_cert_row(header, value).appendTo(table_layout);
>
> But I don't insist on this change.
Changed.
>
>>>> Patch 0024:
>>>>
>>>> Will require changes described in patch 21(rpc).
>>>>
>>>> 1. should the methods have rather params: serial_number and
>>>> revocation_reason, instead of certificate(maybe ok) and dialog(not ok)?
>>>> That way it would be more general/reusable.
> 1. buttons.restore should not be removed, it is still used on
> stageuser.js:309: label: '@i18n:buttons.restore',
The label is returned back.
>
>>>> Patch 0025: ACK
> Why the new revision newly doesn't remove IPA.cert.view_action and
> IPA.cert.get_action implementation ?
I accidentally removed it. Returned back in new revision.
>
>>>> Patch 0026:
>>>>
>>>> 1. Add link use exactly the same code as in multivalued_widget. The code
>>>> should be moved to method: create_add_link(container) and then reused.
>>>> new_row would be overridedn and it would call open_addcert_dialog or
>>>> open_addcert_dialog renamed to new_row.
>>>>
>>>> 2. it can have a spec default:
>>>> spec.dropdown_menu = spec.dropdown_menu !== undefined ? true :
>>>> spec.dropdown_menu;
>>>>
>>>> And then it doesn't have to be defined in patches 28, 29, 30.
> 1. why is there a custom certs_field with:
> spec.adapter = spec.field_adapter || {};
>
> field_adapter was supposed to be used only in section. In field IMHO,
> you can use 'adapter' spec property directly. We can use
> f.register('certs', field.field);
>
> Or am I missing something?
No, you are not. Changed from field_adapter to adapter.
>
> 0027-4: ACK
>
> 0028-3: ACK
Because of change above there is a new revision of patch 0028.
>
>
> Patch 0029-3 and 0030-3:
>
> If 0028 makes IPA.host.has_password_evaluator reusable, then it might be
> good to rename it/move so that it is not called "host".
>
> And then IPA.service.has_keytab_evaluator needs to be removed.
>
I guess that you meant IPA.host.has_keytab_evaluator in first sentence
too. So I hope that this is fixed.
>
>>>
>> I moved big part of certs_widget code to more general
>> custom_command_multivalued_widget. This change will be useful in future.
>>
>> Updated patches attached. It would be nice to apply patch 57 between patches 25
>> and 26, but it should be possible to apply it on the top.
> Patch 52-2: ACK
>
> Patch 57: ACK
>
> 1. I wonder if `entity: that.facet.entity.name` in create_add_command
> will be future-proof, but that can be improved later.
I'm not sure why it is not future-proof. We can discuss it later.
>
>
> In general: I don't like the fact that cert-find commands takes at least
> 1.2s which makes loading of user,host,service details page 6x slower
> than before. From access log, it seems it(or dogtag) does too many
> unnecessary things.
Yes that's true, I'm adding jcholast to CC, I think that he should know
about it.
Updated patches attached.
--
Pavel^3 Vomacka
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pvomacka-0018-3-Add-support-for-custom-menu-in-multivalued-widget.patch
Type: text/x-patch
Size: 4603 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160627/427c4ac3/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pvomacka-0019-3-Extends-functionality-of-DropdownWidget.patch
Type: text/x-patch
Size: 2227 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160627/427c4ac3/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pvomacka-0023-3-Refactored-certificate-view-and-remove-hold-dialog.patch
Type: text/x-patch
Size: 14593 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160627/427c4ac3/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pvomacka-0024-3-Changed-the-way-how-to-handle-remove-hold-and-revoke.patch
Type: text/x-patch
Size: 3513 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160627/427c4ac3/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pvomacka-0025-3-Remove-old-useless-actions-get-and-view.patch
Type: text/x-patch
Size: 3399 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160627/427c4ac3/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pvomacka-0026-4-Add-widget-for-showing-multiple-certificates.patch
Type: text/x-patch
Size: 9868 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160627/427c4ac3/attachment-0005.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pvomacka-0028-4-Add-new-certificates-widget-to-the-user-details-page.patch
Type: text/x-patch
Size: 2405 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160627/427c4ac3/attachment-0006.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pvomacka-0029-4-Add-new-certificates-widget-to-the-host-details-page.patch
Type: text/x-patch
Size: 6607 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160627/427c4ac3/attachment-0007.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pvomacka-0030-4-Add-new-certificates-widget-to-the-service-details-p.patch
Type: text/x-patch
Size: 3640 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160627/427c4ac3/attachment-0008.bin>
More information about the Freeipa-devel
mailing list