[Freeipa-devel] [PATCHES] 0072-0074 Add automember rebuild membership to the web UI

[Petr Vobornik]

On 10/29/2013 12:33 PM, Ana Krivokapic wrote:
> On 10/24/2013 03:48 PM, Petr Vobornik wrote:
>> On 10/16/2013 11:37 PM, Ana Krivokapic wrote:
>>> On 09/27/2013 04:38 PM, Petr Vobornik wrote:
>>>> On 09/25/2013 11:51 AM, Ana Krivokapic wrote:
>>>>> Hello,
>>>>>
>>>>> This patch set addresses ticket
>>>>> https://fedorahosted.org/freeipa/ticket/3928.
>>>>>
>>>>> Patch 0072 hooks the new automember-rebuild command to the web UI
>>>>> (user and host
>>>>> pages).
>>>>> Patch 0073 adds some fixes to the web UI test driver, which are
>>>>> necessary for
>>>>> patch 0074.
>>>>> Patch 0074 adds web UI integration tests for the new feature.
>>>>>
>>>>> The patch set applies on top of my patches 0068-0071
>>>>>
>>>>
>>>> patch 72: Me, Ana and Kyle discussed position of the actions and the
>>>> decision was to move them to action list.
>>>>
>>>> `ipa automember-rebuild --type=hostgroup` can't be run from UI
>>>> (already discussed with Ana as well)
>>>>
>>>> I'm thinking about adding/creating refresh facet policies for this
>>>> action so that appropriate facet would be marked as dirty and
>>>> therefore refreshed so user would not have click association facet
>>>> refresh button.
>>>>
>>>> patch 73: Looks OK but some changes might not be needed.
>>>>
>>>> patch 74: I would use different methods for certain actions to be
>>>> consistent with other tests and to make the test more robust against
>>>> Web UI refactoring:
>>>>
>>>> 1.
>>>>> self.click_on_link('Refresh')
>>>>
>>>> For buttons in .facet-controls use rather
>>>> `self.facet_button_click('refresh')` where 'refresh' is the button
>>>> name, not text.
>>>>
>>>> 2.
>>>>> self.add_record(
>>>>>              'automember',
>>>>>              {'pkey': 'webservers', 'add': [
>>>>>                  ('selectbox', 'key', 'fqdn'),
>>>>>                  ('textbox', 'automemberinclusiveregex', '^web[1-9]+')
>>>>>              ]},
>>>>>              facet='hostgrouprule',
>>>>>              facet_btn_css_class='widget',
>>>>>              navigate=False
>>>>>          )
>>>>
>>>> use `add_table_record('automemberinclusiveregex', data, parent)`.
>>>> Example in test_dns.py:94.
>>>>
>>>> 3. A nitpick(or not even that): When working on one entity, I would
>>>> rather use `navigate_by_breadcrumb('link text') instead of direct
>>>> `navigate_to_record` call which changes url. It resembles user's
>>>> behavior more. But it depends on situation. Ie. when I'm on hostgroup
>>>> page and want to go to host search page, but the last host page was
>>>> some details or association I may just use `navigate_to_entity('host',
>>>> 'search'). Anyway, the important thing is to avoid navigating to the
>>>> same url twice in a row - IE10 does not like that.
>>>>
>>>> 4. Do not use `wait(1)` for AJAX calls. The test will fail if there is
>>>> bigger delay. User rather `wait_for_request(n=X)` where X is number of
>>>> AJAX calls you want to wait for.
>>>>
>>>> 5. Instead of `click_on_link('Rebuild auto membership')` you should
>>>> use `action_panel_action(panel_name, action)` There is also similar
>>>> call for
>>>> executing action list action: `action_list_action(action_name)`.
>>>>
>>>> 6. Nitpick: you can reduce the cleanup code:
>>>>> self.navigate_by_menu('identity/host')
>>>>> self.delete('host', [{'pkey': host1}])
>>>>> self.delete('host', [{'pkey': host2}])
>>>>
>>>> Can be written as:
>>>>
>>>>      self.delete('host', [{'pkey': host1}, {'pkey': host2}])
>>>>
>>>> or
>>>>      self.navigate_by_menu('identity/host')
>>>>      self.delete_record('host', [host1, host2])
>>>>
>>>> `delete` is basically a shortcut for `navigate_to_entity` and
>>>> `delete_record` with CRUD data format.
>>>
>>> All fixed, new patches attached.
>>>
>>
>> Patch 72: ACK
>>
>> Patch 73: existing UI_driver.select method has the same functionality as new
>> select_option method.
>>
>> Patch 74: ACK
>
> Oops, I missed the existing select() method. Thanks for poining it out. Updated
> patch 73 drops the unnecessary select_option() and uses existing select().
>

ACK
-- 
Petr Vobornik