Repository navigation
Conversation
Previously all resources of an application (context) shared a single viewSetting object. Because the per-view localViewSetting chain ends up referencing that same object, entering a search term above one view mutated the shared object and thereby filtered every view in the application at once. Clearing the search once decoupled only that one view (the deep clone in setSearchString), which made the behavior inconsistent depending on interaction order. View settings (search string, filters, hidden columns, sorting) are now kept per resource, keyed by the resource key, and reset when the application is reloaded. Fixes: nextcloud#3008 Assisted-by: pi:glm-5.3-flash Signed-off-by: Alexander R. Krug <krug@coll.mpg.de>
Add a Cypress spec asserting that a search term entered above one view of an application filters only that view while the other views of the application stay uninterrupted, and that clearing the search un-filters it. Guards against a regression of the shared viewSetting object that previously leaked the search across all views (fixes nextcloud#3008). To make the fixture buildable and the assertions robust: - extend the createContext command with optional resource titles, driven through the creation modal's contextResourceForm typeahead - add stable data-cy hooks: contextTableNode / contextViewNode on the context page's per-resource wrappers and elementSearchInput on the shared per-element search input, so the tests can scope to one resource's search input without depending on markup or class order - add the spec to the committed e2e test list - collapse two pre-existing double blank lines in commands.js flagged by lint Signed-off-by: Alexander R. Krug <krug@coll.mpg.de>
626afb4 to
1e2c2b7
Compare
enjeck
left a comment
enjeck
left a comment
There was a problem hiding this comment.
Thank you! I have some comments on the tests
There was a problem hiding this comment.
can we have playwright test instead?
There was a problem hiding this comment.
I'll try. Test setup is what cost me the most time with this PR. I'll do my best to incorporate the other feedback while switching to Playwright. I'll push and ping when I have something for you to look at
There was a problem hiding this comment.
@krugar What's making the setup take time? It should be enough to just download playwright and run npm test:e2e ? Or you mean actually writing the tests? In which case, AI usually gets it right, you just need to nudge it to avoid pitfalls like the things i pointed out.
There was a problem hiding this comment.
@enjeck both PRs updated with Playwright tests (no wait(), independent setup).
As to why test setup took time: I wasn't familiar with the stack and had to do some fiddling to get it running on my system. I'm using pnpm instead of npm, and I'm on a Fedora variant with Chromium installed (only) as Flatpak. There's also were some phantom dependencies in the Cypress setup that your CI seems to get from elsewhere. I can provide details if you care about them, but I don't want to waste your time with them if Cypress is deprecated anyways
| // wait for the search to be applied (debounced) | ||
| cy.wait(700) |
| cy.createContext(contextTitle, false, [viewOneTitle, viewTwoTitle]) | ||
| }) | ||
|
|
||
| it('Search in one view must not filter the other views', () => { |
There was a problem hiding this comment.
this depends on the previous test? It's better when every test is independent so one failing doesn't affect the other.
|
I have also fixed this in #3091 |
Replace the Cypress regression test for the per-view search scope (nextcloud#3008) with a Playwright spec, addressing the review feedback on the PR: - Playwright instead of Cypress - no wait(): the debounced search is synchronized with retrying toHaveCount assertions instead of waitForTimeout - no inter-test reliance: every test builds its own fixture through the per-test userPage fixture (own random user per test), and the suite runs fully parallel The createContext helper in the Playwright support code gains optional resource titles, driven through the creation modal's contextResourceForm typeahead. The data-cy hooks (contextTableNode/contextViewNode/elementSearchInput) are unchanged. The Cypress spec, its createContext extension and the spec-list entry are removed again. Assisted-by: pi:glm-5.3-flash Signed-off-by: Alexander R. Krug <krug@coll.mpg.de>
3 participants
This is meant to fix #3008.
Expanded the view settings object to not be one set of settings shared across all views in a context, but a dictionary keyed by
resource.key. This is meant to allow filtering only one Node in a Context with multiple Nodes present, and to prevent cross-filtering. renamedviewSettingtoviewSettingsto make the intent clear.To discuss: this also makes sorting and filtering an explicitly per-Node action. I'm preparing a parallel PR adding a Context-wide search box (#3056), if Context-wide sorting/filtering is desired, I can try to add that, but would need guidance on UI/UX (e.g.: pull up columns with a technicalName present in all views? But into what kind of UI element?)
I've guessed naming convention for the
cy-dataattributes, I hope in this place,Nodeis correct (could also beElement).🖼️ Screenshots
none
🏁 Checklist
/backport to stableX.X🤖 AI (if applicable)