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

feature: api delete for root documents #25

Merged
merged 9 commits into from
Nov 22, 2024

Conversation

lebalz
Copy link
Contributor

@lebalz lebalz commented Nov 11, 2024

Fix: correctly provide documents when shared access was granted

The sharing-model was not fully implemented and in #23 a bug was introduced, where the user-access had precedence over the the shared permission in case the root access was None.

Fix: ensure the route-guard is applied for admins too!

We somehow introduced that admin access is not checked by the route guard. (Maybe for debugging purpose?) I added a logger option for development to display route-guard infos

Fix/Feature: notify users when new a new document root is created

When dynamic doc roots are created, users must be notified...

Feature: api delete for root documents

When a root document is dynamically generated, an admin should be able to delete it again.
This is needed for GBSL-Informatik/teaching-dev#47

@SilasBerger
Copy link
Contributor

As far as I can tell, this PR includes changes from the other three that are currently open. It is probably best to review this one once the others are merged. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not needed to be reviewed, since a second migration of the view was needed ;)

Comment on lines +110 to +115
CASE
WHEN documents.author_id = rup.user_id THEN rup.access
WHEN document_roots.shared_access = 'RO_DocumentRoot' THEN 'RO_User'
WHEN document_roots.shared_access = 'RW_DocumentRoot' THEN 'RW_User'
ELSE document_roots.shared_access
END AS access,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added: Distinguish between own and shared documents

Comment on lines +129 to +132
(
rup.access >= document_roots.shared_access
AND document_roots.shared_access != 'None_DocumentRoot'
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

+: ensure the shared_access is not None

Comment on lines +165 to +169
CASE
WHEN document_roots.shared_access = 'RO_DocumentRoot' THEN 'RO_StudentGroup'
WHEN document_roots.shared_access = 'RW_DocumentRoot' THEN 'RW_StudentGroup'
ELSE document_roots.shared_access
END AS access,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

+: same for group based access

Comment on lines +177 to +181
ON (
document_roots.id=rgp.document_root_id
AND rgp.access >= document_roots.shared_access
AND document_roots.shared_access != 'None_DocumentRoot'
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

+: and here aswell, don't provide None Access

Comment on lines -103 to -105
if (isAdmin) {
return true;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Huch, shouldn't admins be checked through the guard too!?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is ringing a bell now... I think we decided that admins should just be able to do whatever they want - but I don't think this was a great decision back then :D

Copy link
Contributor

@SilasBerger SilasBerger left a comment

Choose a reason for hiding this comment

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

This looks good, thanks! I didn't find any issues 👍

Copy link
Contributor

@SilasBerger SilasBerger Nov 22, 2024

Choose a reason for hiding this comment

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

Now this actually means something, too 😆

@lebalz lebalz merged commit 2e1510e into main Nov 22, 2024
1 check passed
@lebalz lebalz deleted the feature/api-delete-for-root-documents branch November 22, 2024 15:22
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.

2 participants