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: Deleted time added to the database #903

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

Monirzadeh
Copy link
Collaborator

@Monirzadeh Monirzadeh commented May 8, 2024

This PR will record deleted time with bookmark id in a seprate table.
any specific unit test needed?

Why we need this?

for create a sync functionality we need to have 3 things created_at modifieded_at and deleted_at
the last one added in this PR. as deleted_at use case is just for sync than keep that in separate table make things cleaner.

after merge #894 #896 and #903 i can finally send PR for add sync functionality to Shiori (some part of code is ready in local system). so user can sync their apps with Shiori server.

Copy link

codecov bot commented May 8, 2024

Codecov Report

Attention: Patch coverage is 0% with 30 lines in your changes are missing coverage. Please review.

Project coverage is 30.56%. Comparing base (595cb45) to head (aaf0d16).
Report is 34 commits behind head on master.

Files Patch % Lines
internal/database/mysql.go 0.00% 10 Missing ⚠️
internal/database/pg.go 0.00% 10 Missing ⚠️
internal/database/sqlite.go 0.00% 10 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #903      +/-   ##
==========================================
+ Coverage   25.58%   30.56%   +4.97%     
==========================================
  Files          47       60      +13     
  Lines        5628     4486    -1142     
==========================================
- Hits         1440     1371      -69     
+ Misses       3989     2893    -1096     
- Partials      199      222      +23     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Monirzadeh Monirzadeh requested a review from fmartingr May 8, 2024 23:03
@fmartingr
Copy link
Member

I'm still not sold on this because it changes a foundational thing about Shiori and it may be confusing for users (that delete no longer means delete). I'd rather think in a better way for third-parties to know when some bookmarks are gone. Is knowing when a bookmark has been deleted the only remaining problem?

@Monirzadeh
Copy link
Collaborator Author

Monirzadeh commented May 10, 2024

yes knowing deleted item is the remaining part.
i thinking about different method

  1. normally programs done thumb stone that i don't like that at all. (terrible method) big NO to me 🙅
  2. a way is that user send id that have in client side and we check one by one (that i think it is intensive unless we find a way to done that quickly)
  3. current method is the best way that i found yet that keep minimal possible footprint. not prefect.

client may don't have all bookmarks just have part of it.
for example in a database with 1000 bookmark client maybe have just 100 recently item (900 - 1000) but maybe client has bookmark id 503 (get and save that from tag search for example).
what is the best method to define which one of that 101 (900-1000 plus bookmark id 503) item deleted?

i need to return something like this:

  • bookmarks added after timestamp user send (client last sync time)
  • bookmark modified after timestamp user send
  • list of id that deleted (better after timestamp user send)

one way is that i return any missing id with something like this (it is a quick code and has some bug) what do you think?

WITH RECURSIVE range(id) AS (
  SELECT 1
  UNION ALL
  SELECT id + 1 FROM range WHERE id < (SELECT MAX(id) FROM bookmark)
)
SELECT id
FROM range
WHERE id NOT IN (SELECT id FROM bookmark);

@Monirzadeh Monirzadeh self-assigned this May 17, 2024
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.

None yet

2 participants