On 6/5/2014 9:25 AM, Endi Sukma Dewata wrote:
ACK for patches #592-#628. I'll continue reviewing the rest.

ACK for patches #633-639, #642, #644, #652, and #653. Patches #640 & #641 have an issue (see #19 below) that should be fixed before pushing. Other issues are minor/unrelated/suggestions that can be addressed separately.

13. The separators in the top right drop down menu shouldn't be focusable. To test this, click the menu, then hit tab several times.

14. In the certificates page, if I enter a search filter then hit Refresh (instead of Enter) it doesn't change the content based on the new search filter. I suppose a more intuitive behavior would be to rerun the search with the new filter, or reset the filter to the previous value.

15. In the certificates page, if I enter an invalid filter it will show an error dialog, then after I close it it will show the error message in the page. This is fine, but if I go to another page, then back, it will do the same thing as if the search is executed again. If the page is cached, it probably shouldn't display the error dialog, just the error message in the page. Alternatively, if the search failed it shouldn't be cached.

16. In the association adder dialog it's not clear if the filter applies to just the Available list or the Prospective list too. Maybe the placeholder text could say "Filter available ${other_entity}".

17. It looks like some facet-actions elements contain unnecessary blank ul.dropdown-menu elements which probably correspond to the number of menu items (see User's Actions button).

18. In the New Certificate dialog for Host, the instruction to create a CSR exceeds the dialog boundary.

19. The "View certificate" is missing from the Host's and Service's Action, probably because of the incorrect labels below:

  IPA.cert.view_action = function(spec) {

    // should be objects.cert.view_certificate_btn
    spec.label = spec.label || '@i18n:objects.cert.view';

    that.execute_action = function(facet) {

      // should be objects.cert.view_certificate
      var title = text.get('@i18n:objects.cert.view_certificate_btn');

20. The capitalization of "Certificate" is inconsistent in Host's and Service's Actions.

21. Should there be a link from the Host page to the Certificate page? Can the certificate serial number be displayed in the Host page? If we do this, it's probably not necessary to have Revoke/Restore Certificate actions in the Host page. Same thing with the Service page.

22. The add dialog for RADIUS Server uses a field label "Secret". Everywhere else in the RADIUS Server page it's called "Password" (e.g. Verify Password, Reset Password).

Endi S. Dewata

Freeipa-devel mailing list

Reply via email to