-
Notifications
You must be signed in to change notification settings - Fork 279
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
Implement server-side bookmarks #2843
Merged
+1,040
−321
Merged
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
5aea511
Implement basic CRUD operations for bookmarks
tillprochaska 9002c6e
Delete bookmarks when entity is deleted
tillprochaska a8a5afb
Run Alembic migrations in test environment
tillprochaska f3c4de5
Add endpoint to migrate bookmarks from client-side storage
tillprochaska e778c66
Return whether entity is bookmarked in entity API response
tillprochaska 1c53c8a
Remove warning popover when using bookmarks feature for the first time
tillprochaska 55193dc
Load bookmarks from API
tillprochaska 26798bb
Automatically migrate local bookmarks
tillprochaska 43b6ac1
Extend data fetching logic to support partial invalidations
tillprochaska 5237f5c
Actually commit ORM session to execute queries
tillprochaska ff6a27d
Update wording
tillprochaska File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
41 changes: 41 additions & 0 deletions
41
aleph/migrate/versions/c52a1f469ac7_create_bookmark_table.py
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,41 @@ | ||
"""create bookmark table | ||
|
||
Revision ID: c52a1f469ac7 | ||
Revises: 274270e01613 | ||
Create Date: 2023-01-30 08:53:59.370110 | ||
|
||
""" | ||
|
||
# revision identifiers, used by Alembic. | ||
from alembic import op | ||
import sqlalchemy as sa | ||
|
||
revision = "c52a1f469ac7" | ||
down_revision = "274270e01613" | ||
|
||
|
||
def upgrade(): | ||
op.create_table( | ||
"bookmark", | ||
sa.Column("id", sa.Integer(), primary_key=True), | ||
tillprochaska marked this conversation as resolved.
Show resolved
Hide resolved
|
||
sa.Column("created_at", sa.DateTime(), nullable=False), | ||
sa.Column("role_id", sa.Integer(), sa.ForeignKey("role.id"), nullable=False), | ||
sa.Column("entity_id", sa.String(length=128), nullable=False), | ||
tillprochaska marked this conversation as resolved.
Show resolved
Hide resolved
|
||
sa.Column( | ||
"collection_id", | ||
sa.Integer(), | ||
sa.ForeignKey("collection.id"), | ||
nullable=False, | ||
), | ||
sa.UniqueConstraint("role_id", "entity_id"), | ||
) | ||
|
||
op.create_index( | ||
op.f("ix_bookmark_role_id_collection_id_created_at"), | ||
"bookmark", | ||
["role_id", "collection_id", "created_at"], | ||
) | ||
|
||
|
||
def downgrade(): | ||
op.drop_table("bookmark") |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
from datetime import datetime | ||
from normality import stringify | ||
from aleph.core import db | ||
from aleph.model.common import ENTITY_ID_LEN, IdModel | ||
|
||
|
||
class Bookmark(db.Model, IdModel): | ||
tillprochaska marked this conversation as resolved.
Show resolved
Hide resolved
|
||
"""A bookmark of an entity created by a user.""" | ||
|
||
created_at = db.Column(db.DateTime, default=datetime.utcnow) | ||
role_id = db.Column(db.Integer, db.ForeignKey("role.id")) | ||
collection_id = db.Column(db.Integer, db.ForeignKey("collection.id")) | ||
entity_id = db.Column(db.String(ENTITY_ID_LEN)) | ||
|
||
def to_dict(self): | ||
return { | ||
"id": stringify(self.id), | ||
"created_at": self.created_at, | ||
"entity_id": self.entity_id, | ||
"collection_id": self.collection_id, | ||
} | ||
|
||
@classmethod | ||
def delete_by_entity(cls, entity_id): | ||
query = db.session.query(Bookmark) | ||
query = query.filter(Bookmark.entity_id == entity_id) | ||
query.delete(synchronize_session=False) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,290 @@ | ||
import json | ||
import datetime | ||
from aleph.index.entities import index_entity | ||
from aleph.tests.util import TestCase, JSON | ||
from aleph.model import Bookmark | ||
from aleph.core import db | ||
|
||
|
||
class BookmarksApiTestCase(TestCase): | ||
def setUp(self): | ||
super(BookmarksApiTestCase, self).setUp() | ||
|
||
self.role, self.headers = self.login() | ||
self.collection = self.create_collection(self.role, label="Politicians") | ||
|
||
data = {"schema": "Person", "properties": {"name": "Angela Merkel"}} | ||
self.entity = self.create_entity(data=data, collection=self.collection) | ||
index_entity(self.entity) | ||
|
||
def test_bookmarks_index_auth(self): | ||
res = self.client.get("/api/2/bookmarks") | ||
assert res.status_code == 403, res | ||
|
||
res = self.client.get("/api/2/bookmarks", headers=self.headers) | ||
assert res.status_code == 200, res | ||
assert res.json["total"] == 0, res.json | ||
|
||
def test_bookmarks_index_results(self): | ||
other_role, _ = self.login("tester2") | ||
other_entity = self.create_entity( | ||
data={"schema": "Person", "properties": {"name": "Barack Obama"}}, | ||
collection=self.collection, | ||
) | ||
index_entity(other_entity) | ||
|
||
bookmarks = [ | ||
Bookmark( | ||
entity_id=self.entity.id, | ||
collection_id=self.entity.collection_id, | ||
role_id=self.role.id, | ||
), | ||
Bookmark( | ||
entity_id=other_entity.id, | ||
collection_id=self.entity.collection_id, | ||
role_id=other_role.id, | ||
), | ||
] | ||
db.session.add_all(bookmarks) | ||
db.session.commit() | ||
|
||
res = self.client.get("/api/2/bookmarks", headers=self.headers) | ||
assert res.status_code == 200, res | ||
assert res.json["total"] == 1, res.json | ||
entity = res.json["results"][0]["entity"] | ||
assert entity["properties"]["name"][0] == "Angela Merkel", res.json | ||
|
||
def test_bookmarks_index_order(self): | ||
other_entity = self.create_entity( | ||
data={"schema": "Person", "properties": {"name": "Barack Obama"}}, | ||
collection=self.collection, | ||
) | ||
index_entity(other_entity) | ||
|
||
old_bookmark = Bookmark( | ||
entity_id=self.entity.id, | ||
collection_id=self.entity.collection_id, | ||
role_id=self.role.id, | ||
created_at=datetime.date(2005, 11, 22), | ||
) | ||
new_bookmark = Bookmark( | ||
entity_id=other_entity.id, | ||
collection_id=other_entity.collection_id, | ||
role_id=self.role.id, | ||
created_at=datetime.date(2009, 1, 20), | ||
) | ||
db.session.add_all([old_bookmark, new_bookmark]) | ||
db.session.commit() | ||
|
||
res = self.client.get("/api/2/bookmarks", headers=self.headers) | ||
assert res.status_code == 200, res | ||
assert res.json["total"] == 2, res.json | ||
first = res.json["results"][0]["entity"] | ||
second = res.json["results"][1]["entity"] | ||
assert first["properties"]["name"][0] == "Barack Obama", first | ||
assert second["properties"]["name"][0] == "Angela Merkel", second | ||
|
||
def test_bookmarks_index_access(self): | ||
other_role = self.create_user(foreign_id="other") | ||
secret_collection = self.create_collection(other_role, label="Top Secret") | ||
|
||
data = {"schema": "Person", "properties": {"name": ["Mister X"]}} | ||
secret_entity = self.create_entity(data, secret_collection) | ||
index_entity(secret_entity) | ||
|
||
bookmark = Bookmark( | ||
entity_id=secret_entity.id, | ||
collection_id=secret_entity.collection_id, | ||
role_id=self.role.id, | ||
) | ||
db.session.add(bookmark) | ||
db.session.commit() | ||
|
||
res = self.client.get("/api/2/bookmarks", headers=self.headers) | ||
|
||
assert res.status_code == 200, res | ||
assert res.json["total"] == 0, res.json | ||
assert len(res.json["results"]) == 0, res.json | ||
|
||
def test_bookmarks_create(self): | ||
count = Bookmark.query.count() | ||
assert count == 0, count | ||
|
||
res = self.client.post( | ||
"/api/2/bookmarks", | ||
headers=self.headers, | ||
data=json.dumps({"entity_id": self.entity.id}), | ||
content_type=JSON, | ||
) | ||
assert res.status_code == 201, res | ||
entity = res.json.get("entity") | ||
assert entity["properties"]["name"][0] == "Angela Merkel", res.json | ||
|
||
count = Bookmark.query.count() | ||
assert count == 1, count | ||
bookmark = Bookmark.query.first() | ||
assert bookmark.entity_id == self.entity.id, bookmark.entity_id | ||
assert bookmark.role_id == self.role.id, bookmark.role_id | ||
|
||
def test_bookmarks_create_validate_access(self): | ||
other_role = self.create_user(foreign_id="other") | ||
secret_collection = self.create_collection(other_role, label="Top Secret") | ||
|
||
data = {"schema": "Person", "properties": {"name": ["Mister X"]}} | ||
secret_entity = self.create_entity(data, secret_collection) | ||
index_entity(secret_entity) | ||
|
||
res = self.client.post( | ||
"/api/2/bookmarks", | ||
headers=self.headers, | ||
data=json.dumps({"entity_id": secret_entity.id}), | ||
content_type=JSON, | ||
) | ||
assert res.status_code == 400, res | ||
message = res.json["message"] | ||
assert message.startswith("Could not bookmark the given entity"), message | ||
|
||
count = Bookmark.query.count() | ||
assert count == 0, count | ||
|
||
def test_bookmarks_create_validate_exists(self): | ||
invalid_entity_id = self.create_entity( | ||
{"schema": "Company"}, self.collection | ||
).id | ||
|
||
res = self.client.post( | ||
"/api/2/bookmarks", | ||
headers=self.headers, | ||
data=json.dumps({"entity_id": invalid_entity_id}), | ||
content_type=JSON, | ||
) | ||
assert res.status_code == 400, res | ||
message = res.json["message"] | ||
assert message.startswith("Could not bookmark the given entity"), message | ||
|
||
count = Bookmark.query.count() | ||
assert count == 0, count | ||
|
||
def test_bookmarks_create_idempotent(self): | ||
count = Bookmark.query.count() | ||
assert count == 0, count | ||
|
||
res = self.client.post( | ||
"/api/2/bookmarks", | ||
headers=self.headers, | ||
data=json.dumps({"entity_id": self.entity.id}), | ||
content_type=JSON, | ||
) | ||
assert res.status_code == 201 | ||
|
||
count = Bookmark.query.count() | ||
assert count == 1, count | ||
|
||
res = self.client.post( | ||
"/api/2/bookmarks", | ||
headers=self.headers, | ||
data=json.dumps({"entity_id": self.entity.id}), | ||
content_type=JSON, | ||
) | ||
assert res.status_code == 201 | ||
|
||
count = Bookmark.query.count() | ||
assert count == 1, count | ||
|
||
def test_bookmarks_delete(self): | ||
bookmark = Bookmark( | ||
entity_id=self.entity.id, | ||
collection_id=self.entity.collection_id, | ||
role_id=self.role.id, | ||
) | ||
db.session.add(bookmark) | ||
db.session.commit() | ||
|
||
count = Bookmark.query.count() | ||
assert count == 1, count | ||
|
||
res = self.client.delete( | ||
f"/api/2/bookmarks/{self.entity.id}", headers=self.headers | ||
) | ||
assert res.status_code == 204, res | ||
|
||
count = Bookmark.query.count() | ||
assert count == 0, count | ||
|
||
def test_bookmarks_delete_idempotent(self): | ||
count = Bookmark.query.count() | ||
assert count == 0, count | ||
|
||
res = self.client.delete( | ||
f"/api/2/bookmarks/{self.entity.id}", headers=self.headers | ||
) | ||
assert res.status_code == 204, res | ||
|
||
count = Bookmark.query.count() | ||
assert count == 0, count | ||
|
||
def test_bookmarks_migrate(self): | ||
other_entity = self.create_entity( | ||
data={"schema": "Person", "properties": {"name": "Barack Obama"}}, | ||
collection=self.collection, | ||
) | ||
index_entity(other_entity) | ||
|
||
count = Bookmark.query.count() | ||
assert count == 0, count | ||
|
||
body = [ | ||
{"entity_id": self.entity.id, "created_at": "2005-11-22T00:00:00Z"}, | ||
{"entity_id": other_entity.id, "created_at": "2009-01-20T00:00:00Z"}, | ||
] | ||
res = self.client.post( | ||
"/api/2/bookmarks/migrate", | ||
headers=self.headers, | ||
data=json.dumps(body), | ||
content_type=JSON, | ||
) | ||
assert res.status_code == 201, res | ||
assert res.json["errors"] == [], res.json | ||
|
||
count = Bookmark.query.count() | ||
assert count == 2, count | ||
|
||
bookmarks = Bookmark.query.all() | ||
assert bookmarks[0].entity_id == self.entity.id | ||
assert bookmarks[0].created_at == datetime.datetime(2005, 11, 22), bookmarks[0] | ||
assert bookmarks[1].entity_id == other_entity.id | ||
assert bookmarks[1].created_at == datetime.datetime(2009, 1, 20), bookmarks[1] | ||
|
||
def test_bookmarks_migrate_invalid_entity_id(self): | ||
invalid_entity_id = self.create_entity( | ||
{"schema": "Company"}, self.collection | ||
).id | ||
|
||
body = [ | ||
{"entity_id": self.entity.id, "created_at": "2022-01-01T00:00:00Z"}, | ||
{"entity_id": invalid_entity_id, "created_at": "2022-01-01T00:00:00Z"}, | ||
] | ||
res = self.client.post( | ||
"/api/2/bookmarks/migrate", | ||
headers=self.headers, | ||
data=json.dumps(body), | ||
content_type=JSON, | ||
) | ||
assert res.status_code == 201, res | ||
assert res.json["errors"] == [invalid_entity_id], res.json | ||
|
||
count = Bookmark.query.count() | ||
assert count == 1, count | ||
|
||
bookmarks = Bookmark.query.all() | ||
assert bookmarks[0].created_at == datetime.datetime(2022, 1, 1), bookmarks[0] | ||
assert bookmarks[0].entity_id == self.entity.id | ||
|
||
def test_bookmarks_migrate_required_timestamp(self): | ||
res = self.client.post( | ||
"/api/2/bookmarks/migrate", | ||
headers=self.headers, | ||
data=json.dumps([{"entity_id": self.entity.id}]), | ||
content_type=JSON, | ||
) | ||
assert res.status_code == 400, res |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Currently, this isn’t covered by tests. I was trying to find existing tests for this method without success so far, and I guess it doesn’t make a lot of sense to test
prune_entity
in isolation? Anyone knows whether this is already covered as part of a bigger integration-style test somewhere?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.
Indeed, I'm not seeing any test coverage for this. And I agree it should go into an integration test. Shall we leave a TODO here?