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.

Sign up for GitHub

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

Jump to bottom

[full-ci] Prepare batch actions in contextmenu #5973

Merged
merged 21 commits into from
Nov 19, 2021
Merged

[full-ci] Prepare batch actions in contextmenu #5973

merged 21 commits into from
Nov 19, 2021

Conversation

pascalwengerter
Copy link
Contributor

@pascalwengerter pascalwengerter commented Nov 2, 2021 •
edited by kulmann
Loading

Description

Perhaps could use updated unit/smoke tests

Related Issue

Open tasks

  • finish file actions mixins refactoring from single resources to multiple resources @kulmann

@pascalwengerter pascalwengerter changed the title Prepare batch actions in contextmenu [full-ci] Prepare batch actions in contextmenu Nov 3, 2021
@pascalwengerter pascalwengerter marked this pull request as ready for review November 3, 2021 18:49
@pascalwengerter pascalwengerter force-pushed the 29102021_contextmenu-batch-actions branch from e3291ad to f6b622a Compare November 3, 2021 18:49
@pascalwengerter pascalwengerter changed the title [full-ci] Prepare batch actions in contextmenu Prepare batch actions in contextmenu Nov 3, 2021
kulmann
kulmann previously requested changes Nov 4, 2021
View reviewed changes
Copy link
Contributor

@kulmann kulmann left a comment

Choose a reason for hiding this comment

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

Hmmm. Good as a first solution. But I get the feeling that we should refactor the batch actions into action mixins as well and solve the batch-actions-context-menu inside the ContextActions component. That would also reduce business logic from the BatchActions component, which is always a good idea.

packages/web-app-files/src/components/BatchActions.vue Outdated Show resolved Hide resolved
packages/web-app-files/src/components/BatchActions.vue Outdated Show resolved Hide resolved
packages/web-app-files/src/components/BatchActions.vue Outdated Show resolved Hide resolved
packages/web-app-files/src/components/BatchActions.vue Outdated Show resolved Hide resolved
packages/web-app-files/src/views/Favorites.vue Outdated Show resolved Hide resolved
@pascalwengerter pascalwengerter force-pushed the 29102021_contextmenu-batch-actions branch from f6b622a to e8944e8 Compare November 4, 2021 11:17
@pascalwengerter pascalwengerter mentioned this pull request Nov 5, 2021
Closed
1 task
@kulmann kulmann force-pushed the 29102021_contextmenu-batch-actions branch 3 times, most recently from af2f3d5 to 428f311 Compare November 9, 2021 10:28
@pascalwengerter
Copy link
Contributor Author

@kulmann can be re-based, I cherry-picked the ODS bump commit in #6009

@kulmann kulmann force-pushed the 29102021_contextmenu-batch-actions branch from 4911fb2 to 1578564 Compare November 10, 2021 15:10
@kulmann kulmann changed the title Prepare batch actions in contextmenu [full-ci] Prepare batch actions in contextmenu Nov 10, 2021
@pascalwengerter pascalwengerter mentioned this pull request Nov 11, 2021
Closed
26 tasks
@kulmann kulmann dismissed their stale review November 12, 2021 15:30

worked on the branch myself

@kulmann kulmann force-pushed the 29102021_contextmenu-batch-actions branch 2 times, most recently from 40f7a19 to 9a5d724 Compare November 15, 2021 15:36
@ownclouders
Copy link
Contributor

Results for oC10Files1 https://drone.owncloud.com/owncloud/web/20290/15/1
The following scenarios passed on retry:

  • webUIFilesActionMenu/versions.feature:15

@pascalwengerter
Copy link
Contributor Author

Most recent CI run seems to have a lot of failures with modals not closing anymore, let's dig into that tomorrow!

@kulmann kulmann force-pushed the 29102021_contextmenu-batch-actions branch from 6e65073 to 4793e41 Compare November 16, 2021 08:39
@kulmann
Copy link
Contributor

kulmann commented Nov 16, 2021

Rebased, since it has conflicts in the fileActions.js mixin

@kulmann kulmann force-pushed the 29102021_contextmenu-batch-actions branch 2 times, most recently from 25b8aed to c944748 Compare November 16, 2021 11:20
@ownclouders
Copy link
Contributor

Results for oCISSharingPublic https://drone.owncloud.com/owncloud/web/20309/63/1
The following scenarios passed on retry:

  • webUISharingPublicManagement/shareByPublicLink.feature:19

@kulmann kulmann force-pushed the 29102021_contextmenu-batch-actions branch 2 times, most recently from 0e9cfb4 to 54e986c Compare November 16, 2021 12:48
@ownclouders
Copy link
Contributor

Results for oC10Files1 https://drone.owncloud.com/owncloud/web/20322/15/1
The following scenarios passed on retry:

  • webUIFilesActionMenu/versions.feature:15

@pascalwengerter pascalwengerter force-pushed the 29102021_contextmenu-batch-actions branch from 8bfe63d to 90a9800 Compare November 17, 2021 21:30
@pascalwengerter pascalwengerter force-pushed the 29102021_contextmenu-batch-actions branch from 90a9800 to 5fbe392 Compare November 17, 2021 21:32
fschade
fschade suggested changes Nov 18, 2021
View reviewed changes
Copy link
Contributor

@fschade fschade left a comment

Choose a reason for hiding this comment

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

really great changes, just found some monor things

packages/web-app-files/src/components/AppBar/SelectedResources/BatchActions.vue Outdated Show resolved Hide resolved
packages/web-app-files/src/components/FilesList/ContextActions.vue Outdated Show resolved Hide resolved
packages/web-app-files/src/components/FilesList/ContextActions.vue Outdated Show resolved Hide resolved
packages/web-app-files/src/components/ActionMenuItem.vue Outdated Show resolved Hide resolved
packages/web-app-files/src/mixins/actions/acceptShare.js Show resolved Hide resolved
packages/web-app-files/src/mixins/actions/favorite.js Show resolved Hide resolved
packages/web-app-files/src/mixins/actions/fetch.js Outdated Show resolved Hide resolved
packages/web-app-files/src/mixins/actions/fetch.js
window.open(url, '_blank')
return
}

const url = this.$client.helpers._webdavUrl + file.path
// FIXME: use presigned URL instead
Copy link
Contributor

Choose a reason for hiding this comment

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

is this something for later or should happen in this pr?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure about the implications, @kulmann ?

Copy link
Contributor

Choose a reason for hiding this comment

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

to get this merged it say keep the fixme comment here and do it later

packages/web-app-files/src/mixins/actions/fetch.js Show resolved Hide resolved
packages/web-app-files/tests/unit/components/AppBar/SelectedResources/BatchActions.spec.js Show resolved Hide resolved
fschade
fschade approved these changes Nov 19, 2021
View reviewed changes
Copy link
Contributor

@fschade fschade left a comment

Choose a reason for hiding this comment

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

🚀 🎸 - LGTM

@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

Bug B 1 Bug
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 3 Code Smells

17.6% 17.6% Coverage
0.5% 0.5% Duplication

@pascalwengerter pascalwengerter merged commit f5e1b8a into master Nov 19, 2021
@delete-merged-branch delete-merged-branch bot deleted the 29102021_contextmenu-batch-actions branch November 19, 2021 11:54
@individual-it individual-it mentioned this pull request Nov 23, 2021
Closed
@SwikritiT SwikritiT mentioned this pull request Nov 23, 2021
Merged
10 tasks
phil-davis added a commit that referenced this pull request Dec 2, 2021
@phil-davis
Merge pull request #6047 from owncloud/restore-deleted-tests
0bf39e8 
[tests-only][full-ci]Add valid tests that were removed by PR `#5973`
This was referenced Dec 3, 2021
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kulmann kulmann kulmann left review comments

@individual-it individual-it individual-it left review comments

@fschade fschade fschade approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Batch action Delete makes no sense in shared with and favorite views rightclick for multiple selected files
5 participants
@pascalwengerter @ownclouders @kulmann @individual-it @fschade