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

feat: Saving file #72

Merged
merged 1 commit into from
Jul 23, 2024
Merged

feat: Saving file #72

merged 1 commit into from
Jul 23, 2024

Conversation

hweihwang
Copy link
Contributor

No description provided.

@hweihwang hweihwang self-assigned this Jul 16, 2024
@hweihwang hweihwang linked an issue Jul 16, 2024 that may be closed by this pull request
@hweihwang hweihwang requested a review from juliusknorr July 16, 2024 12:03
@hweihwang hweihwang linked an issue Jul 16, 2024 that may be closed by this pull request
.env.example Outdated
@@ -7,6 +7,8 @@ TLS=false
TLS_KEY=
TLS_CERT=
JWT_SECRET_KEY=your_secret_key
WHITEBOARD_SHARED_SECRET=your_shared_secret
Copy link
Member

Choose a reason for hiding this comment

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

Do you think we could just use the JWT_SECRET_KEY as it is shared already?

if (!roomUsers.has(roomId)) {
return null
}
// TODO: Check if the user has write permissions
Copy link
Member

Choose a reason for hiding this comment

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

Would be still nice to address before merging.

}

$userFolder = $this->rootFolder->getUserFolder($user->getUID());
$file = $userFolder->getById($fileId)[0];
Copy link
Member

Choose a reason for hiding this comment

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

There are a few things we should check here when getting the file, best see

https://github.com/nextcloud/text/blob/74aa530fb4ca922d3577b64cc01676d28c449988/lib/Service/DocumentService.php#L496-L524

for all edge cases that may occur 🙈

if (!$user) {
return new DataResponse(['message' => 'Invalid user'], Http::STATUS_BAD_REQUEST);
}

Copy link
Member

Choose a reason for hiding this comment

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

We probably want to call setVolatileActiveUser to make sure the write is also in scope of the user (e.g. for creating versions and activity entries).

Copy link
Member

@juliusknorr juliusknorr left a comment

Choose a reason for hiding this comment

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

A few comments but generally looks fine 👍

@hweihwang hweihwang requested a review from juliusknorr July 22, 2024 12:04
@hweihwang hweihwang force-pushed the feat/saving-to-file branch from 96ffc78 to fa14485 Compare July 22, 2024 12:35
Signed-off-by: Hoang Pham <[email protected]>

feat: Saving file

Signed-off-by: Hoang Pham <[email protected]>

feat: Saving file

Signed-off-by: Hoang Pham <[email protected]>

feat: Saving file

Signed-off-by: Hoang Pham <[email protected]>
@juliusknorr juliusknorr force-pushed the feat/saving-to-file branch from fa14485 to 408f158 Compare July 23, 2024 13:51
@juliusknorr
Copy link
Member

@hweihwang I squashed your commits into one. ;)

Looks all nice and saving is smooth also with sharing involved.

@juliusknorr juliusknorr merged commit 56aa568 into main Jul 23, 2024
22 checks passed
@juliusknorr juliusknorr deleted the feat/saving-to-file branch July 23, 2024 13:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Saving issues Opening without proper backend connection should fail
2 participants