On 06/27/2016 02:55 PM, Pavel Vomacka wrote:
>>>>>>>>
>>>>> 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:
>>>>>

snip

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

ACK

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

ACK

snip

>>>>>
>>>>> 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:
>>>>>

snip

>> 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:

snip

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

ACK


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

ACK

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

ACK

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

ACK, but please mention additional changes in mail, it's sort of
surprise to see load_revocation_reason removed.


>> 0027-4: ACK
>>
>> 0028-3: ACK
> Because of change above there is a new revision of patch 0028.

0028-4: ACK

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

0029-4 ACK
0030-4 ACK


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

So all is ACKed

master:
* e3e83272c9ffaf2a09f910e754c4a0421c816fd0 Add support for custom menu
in multivalued widget
* f243bd2d6564dd35ab5d506578f5d1d2ccb99b2f Extends functionality of
DropdownWidget
* 3d61aca6237852908e0857f9e28ed9c7243b7a3e Add working widget
* 044d3c25de6806b68b1f1627863873ecb1a87957 Add ability to turn off
activity icon
* e7a55ef30b252a616f50c58f99a538e9b090037c Add Object adapter
* 06a9a84876345135acac933aca9ada235651997e Refactored certificate view
and remove hold dialog
* 260a00b81ff10dbe05d35741febfffde2e4ef1dc Changed the way how to handle
remove hold and revoke actions
* 3056f349b94eb99deb665937a61204e0bfea6b78 Remove old useless actions -
get and view
* 6d3622c600a82f889e77809c982d996974335e62 Add widget for showing
multiple certificates
* 55a0baf1c32e1c472efe2ce81870e05abccb5a4a Add certificate widget
* 0b72571c5ab36dc0a2d93ded9a10a1b7b08d552e Add new certificates widget
to the user details page
* 79ec965a967ee561fe3ad1363a64fbb06702e358 Add new certificates widget
to the host details page. Also extends evaluator and add support for
adapters.
* 82e69e430009d8b47e45dadd6895af1d71ecd1dd Add new certificates widget
to the service details page
* 2f048224d2509cddf07532f9703aa88f4eebc9b8 Updated certificates table
* d7898ac2eb3b9d7b0e24579c9d8ea2f541f55268 Add new custom command
multivalued widget
-- 
Petr Vobornik

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to