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.

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

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


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


>>>
>>> Patch 0025: ACK

Why the new revision newly doesn't remove IPA.cert.view_action  and
IPA.cert.get_action implementation ?

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


0027-4: ACK

0028-3: 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 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.


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.

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