Use AccessControl.requestmethod.buildfacade

[tschorr]

... which will be publicly visible if/after https://github.com/zopefoundation/AccessControl/pull/70 gets merged.

[tschorr]

The one failing robot test on Jenkins looks unrelated.

[tschorr]

@mauritsvanrees @pbauer tells me he's currently unavailable - could you review this?

[mauritsvanrees]

It is not a failing robot test, but a browser test. And it does not seem flaky or unrelated. When I checkout AccessControl master and use the plone.protect branch from this PR, the following test reliably fails:

bin/test -s Products.CMFPlone -m test_controlpanel_browser_usergroups -t test_groups_modify_roles

And it reliably passes for me on current coredev 5.2 without those two changes.

So it does not seem good to merge this at this point.

A possible step to make it slightly easier to debug, is to change this PR to try to import the new buildFacade and fall back to the old _buildFacade. That should be safe to merge, and then we can see if we can use the newer AccessControl.

[tschorr]

@mauritsvanrees interesting. I'm running the same test with the same branches and it passes. I'd prefer sorting this out to falling back to the old import. In what Python versions do you see the failures?

[mauritsvanrees]

That is Python 2.7.15 on Mac OS 10.13.6.

[tschorr]

@mauritsvanrees and could you post a traceback?

[mauritsvanrees]

Same as on Jenkins:

Failure in test test_groups_modify_roles (Products.CMFPlone.controlpanel.tests.test_controlpanel_browser_usergroups.UserGroupsControlPanelFunctionalTest)
Traceback (most recent call last):
  File "/usr/local/Cellar/python@2/2.7.15/Frameworks/Python.framework/Versions/2.7/lib/python2.7/unittest/case.py", line 329, in run
    testMethod()
  File "/Users/maurits/community/plone-coredev/5.2/src/Products.CMFPlone/Products/CMFPlone/controlpanel/tests/test_controlpanel_browser_usergroups.py", line 300, in test_groups_modify_roles
    self.assertEqual(ctrl._value, 'Site Administrator')
  File "/usr/local/Cellar/python@2/2.7.15/Frameworks/Python.framework/Versions/2.7/lib/python2.7/unittest/case.py", line 513, in assertEqual
    assertion_func(first, second, msg=msg)
  File "/usr/local/Cellar/python@2/2.7.15/Frameworks/Python.framework/Versions/2.7/lib/python2.7/unittest/case.py", line 506, in _baseAssertEqual
    raise self.failureException(msg)
AssertionError: u'Contributor' != 'Site Administrator'

[tschorr]

The test code expects the role 'Site Administrator' in position 1 of the result list, however it is actually on position 6. This has been changed in the python3 branch of Products.CMFPlone already (independently of this PR).

Bottom line, the 'Site Administrator' role is in both test results, although in different positions. So the question is now whether order matters - if yes, we need to track where+when the order changed. If not, we should maybe make the test more robust. In any case this PR and the corresponding changes in AccessControl.requestmethod look unrelated to me (other changes in AccessControl master might still be related).

[icemac]

AccessControl 4.0b6 has been released, so it can be required in setup.py of the package.

[mauritsvanrees]

Ah, right, the order has changed. So this PR is good, but it can't be merged until the failing test is taken care of.

I think the test simply tries to select a different role, and checks that this change is persisted. Which role that is, should not matter. So we could start with index=0 and iterate until we find a role which is not selected.