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

Add tests for versions actions #43768

Merged
merged 9 commits into from
Feb 27, 2024

Conversation

artonge
Copy link
Contributor

@artonge artonge commented Feb 22, 2024

The PR initial goal was to tests that permissions were properly checked when triggering versions actions on received shared files. But I wrote some tests utils on the way, and encountered a few bugs.

Tests changes

  • Add utils to e2e tests files_sharing
  • Add move and copy utils for e2e tests of files
  • Add test selectors on files_sharing and files_versions components

Code changes

  • Prevent error in Version.vue from @nc/vue when deleting an NcListItem from one of its actions
  • Remove legacy share permissions editor
  • Init ViewOnlyPlugin after auth to prevent error when using Basic auth

@artonge artonge force-pushed the artonge/tests/add_tests_for_versions_actions branch from 8833278 to c122250 Compare February 22, 2024 17:42
@artonge artonge added this to the Nextcloud 29 milestone Feb 22, 2024
@artonge artonge self-assigned this Feb 22, 2024
@artonge artonge force-pushed the artonge/tests/add_tests_for_versions_actions branch 4 times, most recently from bf9d05e to 6e4b010 Compare February 26, 2024 18:54
@artonge artonge marked this pull request as ready for review February 26, 2024 18:55
@artonge artonge added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Feb 26, 2024
apps/dav/lib/Server.php Dismissed Show dismissed Hide dismissed
@artonge artonge force-pushed the artonge/tests/add_tests_for_versions_actions branch 3 times, most recently from 5c4add8 to 6870c67 Compare February 26, 2024 20:11
@artonge artonge force-pushed the artonge/tests/add_tests_for_versions_actions branch from 6870c67 to f7a0246 Compare February 27, 2024 08:28
Copy link
Member

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

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

😍

@artonge
Copy link
Contributor Author

artonge commented Feb 27, 2024

/backport 5137757 to stable28

@artonge
Copy link
Contributor Author

artonge commented Feb 27, 2024

/backport 5137757 to stable27

@artonge
Copy link
Contributor Author

artonge commented Feb 27, 2024

/backport 5137757 to stable26

@artonge
Copy link
Contributor Author

artonge commented Feb 27, 2024

/backport 5137757 to stable25

Comment on lines +274 to +278
async deleteVersion() {
// Let @nc-vue properly remove the popover before we delete the version.
// This prevents @nc-vue from throwing a error.
await this.$nextTick()
await this.$nextTick()
Copy link
Member

Choose a reason for hiding this comment

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

I feel like we could have something better here 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess we should prevent the property access in @nc/vue, but I was too lazy.

@skjnldsv skjnldsv requested review from a team, szaimen, sorbaugh and emoral435 and removed request for a team February 27, 2024 10:26
Copy link
Contributor

@susnux susnux left a comment

Choose a reason for hiding this comment

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

Awesome!

I still do not like the data-test... selectors, because that is testing implementation instead of user flow (a user does not look for selectors but for semantic like labels or roles). But I think this is the best we can get here :)

@skjnldsv skjnldsv merged commit 455a209 into master Feb 27, 2024
161 checks passed
@skjnldsv skjnldsv deleted the artonge/tests/add_tests_for_versions_actions branch February 27, 2024 14:09
@skjnldsv
Copy link
Member

I still do not like the data-test... selectors,

Seconded, didn't we talked about this 🙈

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

Successfully merging this pull request may close these issues.

3 participants