-
Notifications
You must be signed in to change notification settings - Fork 30
Enhancement: Make tables import asynchronous #1801
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
base: main
Are you sure you want to change the base?
Conversation
5cbf833
to
3c2422c
Compare
Please keep in mind, existing API shall remain stable, but may be flagged deprecated. |
9b0c3bd
to
1dbdfe8
Compare
@blizzz What exactly do you mean? Do not change already existent endpoint (keep it synchronous) but introduce v2/v3 for async tasks (even if this endpoints will be not in use on FE side)? |
Yes, exactly. The old ones can get the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
replied to questions
c0a8bee
to
1c49d2c
Compare
Signed-off-by: Kostiantyn Miakshyn <[email protected]>
1c49d2c
to
599fecd
Compare
@blizzz I've reverted all changes to old endpoints. So they are completely untouched. Now we have also Also it is possible to import uploaded files (app data used under the hood) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is shaping up nicely! It looks like a file was not committed. Did not test yet.
namespace OCA\Tables\Activity; | ||
|
||
use OCA\Tables\AppInfo\Application; | ||
use OCP\Activity\Exceptions\UnknownActivityException; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The underlying file does not seem to be committed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this exception should be located somewhere in the core. I've just stolen code to publish/parse activity from another module
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, indeed. But this is available only as of NC 30, on main we still support 29.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, how to deal with that? Can we just bump minimum NC supported version to 30? Like in other modules: Collectives, Forms, etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We weed need to branch-off, which of course is possible. We were aiming for September originally. Related changes (version bump and housekeeping) would happen in a separate PR. I'll get back about it.
Right now module not ready to import large tables. This PR improves import flow by moving it into asynchronous job.
🗒️ TODO:
🔍 Preview
nextcloud-tables-2025-05-26_18.14.20.mp4