[Freeipa-devel] [PATCH] 0018-0030 webui: add support for more certificates

[Petr Vobornik]

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.

Patch 0019:

1. Shouldn't disable_item or enable_item automatically rerender the items?

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

Patch 0022:

1. Leftover line:
  var sn = record[i].serial_number;

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?

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.

Patch 0025: 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.


Patch 0027:

1. Why is download action name a 'that.data_url' ?

2. the dropdown_menu -> custom_actions change

3. "that.certificate &&" is not required it will be always true
        that.certificate = $.extend({}, certificate);

        if (that.certificate && that.certificate.revoked !== 'undefined') {
            that.get_cert_revoke_status();
        }

4. update_link is weird name for a method which updates data in a table

5. "that.update_link = function(r)", "r" shouldn't be there

6. Following doesn't look future proof:
"""
    that.get_cert_revoke_status = function() {
        // Double check whether the certificate is issued by our CA.
        var issuer = 'CN=' + that.ipa_issuer_commonname + ',' +
            that.ipa_issuer_subjbase;
        if (that.certificate.issuer !== issuer) return;
"""
Would check it with Fraser

7. in handle_revocation_reason, useless call:
  var dd_menu = that.get_dropdown_menu();

8. in compose_dialog_title, there is a check for `cert.subject` if true
it sets `cn`, `o`. But if false then the values are `undefined` which
are then used in the r.replace calls. Is it intended?

Patch 0028:
Looks OK, "dropdown_menu: true" doesn't have to be there as mentioned
earlier

Will test later.

Patch 0029:

1. What about extending value_state_evaluator with adapter and param
property. So that it would use standard adapter by default and every
child could use it's own? IMO other parts of UI could use it as well.
The reason why it doesn't use adapter already is that adapters were
introduced later.

It would simplify both definitions + the definitions in patch 30.

Patch 0030:

Same as patch 0029.
-- 
Petr Vobornik