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

Sharing files to cards #2638

Merged
merged 36 commits into from
Jan 4, 2021
Merged

Sharing files to cards #2638

merged 36 commits into from
Jan 4, 2021

Conversation

juliusknorr
Copy link
Member

@juliusknorr juliusknorr commented Dec 8, 2020

Implements #564

Requires nextcloud/server#24605

  • Collaboration search provider (might be replaced with a modal to pick the card)
  • Share provider
  • Keep compatibility to API endpoints
    • List attachments
    • Create attachment
    • Update attachment
    • Delete attachment

Frontend

  • File picker
  • Upload files

@stefan-niedermann
Copy link
Member

Propertly handle existing attachments

Have you already decided whether or not all existing attachments will be completely migrated to files?

This is important to know for us because in the case we can safely assume that each attachment has a file Id which we can use for thumbnail generation etc.

If existing attachments will not be migrated, we would need to check and we were only able to use thumbnails for newly added images... not so nice 😉

@juliusknorr
Copy link
Member Author

No final decision taken on that yet, but i think it would make most sense to just migrate all the attachments to the files app. The only concern about this on the server side is that we cannot do this during upgrade as depending on the number of files that are around, this might cause a quite long downtime, so we'd need to to this afterwards in a background job. However that would of course imply that there is a migration period, where old attachments are still not migrated.

In any case I'll make the API response different in terms that the type property of attachments will no longer be deck_file but file for those who are in the files app, meaning that you could for now only fetch previews for those. Does this make sense?

@stefan-niedermann
Copy link
Member

In any case I'll make the API response different in terms that the type property of attachments will no longer be deck_file but file for those who are in the files app, meaning that you could for now only fetch previews for those. Does this make sense?

Not really IMHO because we get the information implicitely with the fileId property, no? If it ain't there, it is a "old" deck file.

I am concerning that the proposed way will create a historical dept for infinite time because there is no deterministic "end date" for this intermediate state where a file can or can not be from Deck.

I understand the issue of a long running migration. Maybe your proposal can be an intermediate solution (with a recommendation to migrate the files via the cron job or so), and define a fix version (e.g. 2.5) where the files should be migrated for most users and all users who are left at this point will be migrated hard while updating the Deck app?

@juliusknorr
Copy link
Member Author

Not really IMHO because we get the information implicitely with the fileId property, no? If it ain't there, it is a "old" deck file.

The difference there is that the API was initially designed to also support other types of "attachments", the usecase i had in mind back then was to for example allow attaching a github ticket to a card. That is why I'd still take the type as a criteria for separation.

I am concerning that the proposed way will create a historical dept for infinite time because there is no deterministic "end date" for this intermediate state where a file can or can not be from Deck.

I understand the issue of a long running migration. Maybe your proposal can be an intermediate solution (with a recommendation to migrate the files via the cron job or so), and define a fix version (e.g. 2.5) where the files should be migrated for most users and all users who are left at this point will be migrated hard while updating the Deck app?

True, I think we can make this a fixed date migration where we then just notify the admin in case the attachments are still not migrated at that time, just ignore those in the API then and offer an occ command for manual migration. Then you'd no longer need to care about those. Since we are heading to 1.3 now I'd say just deprecating the old attachments now and dropping in a 2.0 release once it approaches would be fine, we can of course always do this earlier or in 1.5 if no major bump comes up.

@stefan-niedermann
Copy link
Member

Thank you for reevaluating this, sounds like a good plan 🙂

@jakobroehrl
Copy link
Contributor

jakobroehrl commented Dec 29, 2020

I did some testing:

New Tests: @juliushaertl

  • "Show in files" takes me to: http://localhost/server21/index.php/index.php/f/162 (index.php 2x)
  • "Unshare file" has no effect
  • Sharing a file to a deck card in the Files App: the card name entry is always two times in the sharing list: solved with a modal dialog

@juliusknorr

This comment has been minimized.

@juliusknorr juliusknorr force-pushed the enh/files branch 2 times, most recently from 1a8c9b5 to 260fdf3 Compare December 30, 2020 12:05
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr juliusknorr marked this pull request as ready for review January 4, 2021 21:10
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr
Copy link
Member Author

Let's get this in so we can get a first beta out.

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