Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes proper pat-plone-modal config to show user addition info #4048

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

rohnsha0
Copy link
Contributor

@rohnsha0 rohnsha0 commented Nov 7, 2024

fixes #4044

@rohnsha0 rohnsha0 requested review from pbauer and petschki November 7, 2024 19:52
@mister-roboto
Copy link

@rohnsha0 thanks for creating this Pull Request and helping to improve Plone!

TL;DR: Finish pushing changes, pass all other checks, then paste a comment:

@jenkins-plone-org please run jobs

To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass, but it takes 30-60 min. Other CI checks are usually much faster and the Plone Jenkins resources are limited, so when done pushing changes and all other checks pass either start all Jenkins PR jobs yourself, or simply add the comment above in this PR to start all the jobs automatically.

Happy hacking!

Copy link
Member

@petschki petschki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As @pbauer mentioned in the issue, please use only disableAjaxFormSubmit: true and remove redirect* options ... otherwise these interfere.

Notes:

What I didn't know is, that the addform redirects with the searchstring=<newusername> URL query parameter, so the list shows only the added user. while this is nice if you have activated "many users" its maybe confusing when you have only 10 users and after adding a new one the list shows only one user ... we could think of adding searchtring only if many_users is activated?

And please do the same for usergroups_groupsoverview.pt with the additional content options, so that the tabs are not shown:

data-pat-plone-modal='{
  "content": "#content-core",
  "actionOptions": {
    "disableAjaxFormSubmit": true
  }
}'

@petschki
Copy link
Member

PS: and don't forget a news entry for the changelog

@rohnsha0
Copy link
Contributor Author

@jenkins-plone-org please run jobs

@rohnsha0 rohnsha0 requested a review from petschki November 11, 2024 16:47
@rohnsha0
Copy link
Contributor Author

Elements states '['detached']' (list) should contain '['visible']' (list)

what does the above error signify? when running locally it works!

@petschki
Copy link
Member

petschki commented Nov 12, 2024

This test checks, that if the form data isn't valid the modal should remain open with the error message and the invalid form. This is indeed broken, because disableAjaxFormSubmit always closes the modal and redirects to the invalid form in the main browser.

You have to make sure, that you run this robottest scenario with:

$ bin/rfbrowser init chromium
$ ROBOT_BROWSER=headlesschrome bin/test --all -t "user overlay remains on wrong data"

Now this is odd because (at the time of writing this) we cannot use an ajax modal form and show the success message in the main window but stay for errors in the modal at the same time. The only solution would be to remain in the modal on success and on error and the admin has to close the modal manually. (Side effect: the userlist shows up in the modal ... that's not nice 🤨 )

pat-plone-modal has the option displayInModal which defaults to true, that means after you have successfully submitted a form, you remain inside the modal. If you set this to false, the pattern should "keep" the success message and show it in the main window after reloading it.

@rohnsha0
Copy link
Contributor Author

Sorry, but I am unsure how to run robottest using that mentioned scenarious...

@stevepiercy
Copy link
Contributor

It looks like we need to enhance the Test locally section of the docs with when and how to run rfbrowser.

Anyway, @rohnsha0 if you take a look at how Jenkins runs its CI, you might find something useful in its console. It's only a machine that runs the same tests that us mere mortals manually run.

@stevepiercy
Copy link
Contributor

@rohnsha0, @petschki just created a PR with instructions, and I merged it. Instructions should appear at https://6.docs.plone.org/contributing/core/#test-locally

@rohnsha0
Copy link
Contributor Author

You have to make sure, that you run this robottest scenario with:

I ran the tests locally and got following output

  Ran 2 tests with 0 failures, 0 errors, 0 skipped in 2.947 seconds.
Tearing down left over layers:
  Tear down Products.CMFPlone.testing.CMFPloneLayer:Acceptance in 0.000 seconds.
  Tear down plone.testing.zope.WSGIServer in 0.002 seconds.
  Tear down plone.app.robotframework.remote.CMFPloneRobotRemoteLibrary:RobotRemote in 0.000 seconds.
  Tear down Products.CMFPlone.testing.ProductsCMFPloneLayer in 0.018 seconds.
  Tear down plone.app.contenttypes.testing.PloneAppContenttypes in 0.001 seconds.
  Tear down plone.app.testing.layers.PloneFixture in 0.008 seconds.
  Tear down plone.testing.zope.Startup in 0.001 seconds.
  Tear down plone.testing.zca.LayerCleanup in 0.000 seconds.

@rohnsha0
Copy link
Contributor Author

@rohnsha0, @petschki just created a PR with instructions, and I merged it. Instructions should appear at https://6.docs.plone.org/contributing/core/#test-locally

thanks for the efforts @petschki @stevepiercy

@rohnsha0
Copy link
Contributor Author

rohnsha0 commented Dec 6, 2024

@jenkins-plone-org please run jobs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adding a user in classic does not yield any feedback
4 participants