nextcloud / integration_mattermost

Mattermost integration in Nextcloud
GNU Affero General Public License v3.0
19 stars 6 forks source link

Migrate file actions for NC 28 #28

Closed julien-nc closed 1 year ago

julien-nc commented 1 year ago

Migrate all file actions related stuff to work with NC 28.

This was harder than expected because of the OAuth redirection stuff. If a popup is used, no problem, we don't loose the context. But if no popup is used and we get redirected back to NC, we need to get the information on the files that were previously selected to immediately open the send modal.

Before the redirection we need to store the current directory. On page load, after the final redirection to NC, we have the list of file IDs (stored on the server side) and we need the names, types and sizes.

We could also store all the files info in the backend before triggering the redirection. I think the webdav solution is more elegant.

PS: This shows most of what's possible with @nextcloud/files and can be used as an example to migrate other apps.

kyteinsky commented 1 year ago

We could also store all the files info in the backend before triggering the redirection. I think the webdav solution is more elegant.

yup, totally agree with that.

Could not test the app functionality-wise yet but the context menu is there in all its glory.
A small issue: both the file actions for multiple files and single files are visible in the triple dot menu. I think this can be solved by combining and using both the exec and execBatch functions inside one file action and then would executes the functions as per the location of the action: checkbox+top bar button = execBatch, triple dot menu+context menu button = exec.

There is also an error on clicking the send button on the top bar (execBatch), and this may be a bug on the server side. I can't find any clues in this repo:

ConsoleLogger.js:74 [ERROR] files: Error while executing action {app: 'files', uid: 'admin', level: 0, action: Dt, e: TypeError: s.some is not a function

TypeError: s.some is not a function
    at https://nextcloud.local/dist/files-main.js:2:209125
    at u (https://nextcloud.local/dist/files-main.js:2:201336)
    at Generator.<anonymous> (https://nextcloud.local/dist/files-main.js:2:202692)
    at Generator.next (https://nextcloud.local/dist/files-main.js:2:201699)
    at za (https://nextcloud.local/dist/files-main.js:2:207346)
    at s (https://nextcloud.local/dist/files-main.js:2:210062)

Other than that, everything is fine :tada:

julien-nc commented 1 year ago

I think this can be solved by combining and using both the exec and execBatch functions

@kyteinsky I wanted to have different labels but yeah it seems not possible to avoid having the multi action in the single file context menu. All good with one action defining both exec and execBatch :+1: .

There is also an error on clicking the send button on the top bar (execBatch), and this may be a bug on the server side

Yep, bug in @nextcloud/files.

julien-nc commented 1 year ago

@kyteinsky Ready for re-re-review

github-actions[bot] commented 1 year ago

Hello there, Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!