feat: store created and modified time separately on database for bookmarks #896

Monirzadeh · 2024-05-01T09:29:04Z

this PR should done two things:

add created time to the database
modified update if write new content in an exist bookmark.
any place you will done change in any aspect of bookmark (new archive,content,epub,tags,etc)

in future if we want to update modified time any place in code just set book.modified = "" shiori automatically will update modified time.

TODO:

sqlite migrate
update model
fix logic of update modified time
show added time in content page correctly
repeat that for postgres database
change mariadb update modfied_at same as other database mariadb update modified time when row change that not possible in other database this change in this PR and all use same method now.
repeat that for mariadb database
add unit tests

…ppen

codecov · 2024-05-01T09:32:47Z

Codecov Report

Attention: Patch coverage is 80.39216% with 10 lines in your changes missing coverage. Please review.

Project coverage is 35.11%. Comparing base (a3d4a68) to head (19b9221).

❗ Current head 19b9221 differs from pull request most recent head 7db5cd9

Please upload reports for the commit 7db5cd9 to get more accurate results.

Files	Patch %	Lines
internal/cmd/import.go	0.00%	4 Missing ⚠️
internal/cmd/pocket.go	0.00%	4 Missing ⚠️
internal/cmd/export.go	0.00%	1 Missing ⚠️
internal/webserver/handler-api.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #896      +/-   ##
==========================================
+ Coverage   34.73%   35.11%   +0.38%     
==========================================
  Files          61       61              
  Lines        4054     4061       +7     
==========================================
+ Hits         1408     1426      +18     
+ Misses       2424     2413      -11     
  Partials      222      222

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

…add-created-time

fmartingr · 2024-05-06T18:49:21Z

Hey @Monirzadeh is this ready for review? I see items pending.

A side comment for me, would it me much effort renaming the modified column to modified_at and use created_at as creation date column?

Monirzadeh · 2024-05-06T19:04:07Z

mysql is not ready yet
when i fill created with modified value (in maraidb) created (around one or two) second different with modified and i can't find why it happen yet.

ok i will change names column to modified_at and created_at
should i change that in API too or keep that untouch?

fmartingr · 2024-05-06T20:23:56Z

mysql is not ready yet when i fill created with modified value (in maraidb) created (around one or two) second different with modified and i can't find why it happen yet.

Let me know if you get stuck with this, I can take a look if you want.

ok i will change names column to modified_at and created_at should i change that in API too or keep that untouch?

Leave the old API as is, we will use the new field in the new API.

Monirzadeh · 2024-05-07T09:09:02Z

internal/database/migrations/sqlite/0003_created_time.up.sql

+RENAME COLUMN modified to created_at;
+
+ALTER TABLE bookmark
+ADD COLUMN modified_at TEXT NULL;


modified_at is TEXT NULL not

modified_at TEXT NOT NULL DEFAULT CURRENT_TIMESTAMP,

dose it make any problem?

Monirzadeh · 2024-05-07T13:02:55Z

Visual change in this PR
Before

After created time and modified time is in same day

After created time and modified time is not in same day

Monirzadeh · 2024-05-07T13:09:40Z

Hi @fmartingr

as i see all test pass right now. do we need any specific unit test?
about MariaDB i can't done all of them in one migrate (get syntax error while migrate) but when i separate them to multiple stage it work smoothly.
in MariaDB change modified time had different method in compare to postgress and sqlite. this PR change that and now all database update modified_at same each other.
that dark layer in top of screenshot is related to my browser setup not Shiori change. (created time position change on those screenshot)

fmartingr

Hey @Monirzadeh, good job as ususal. I've left a few comments around for changes.

Also, I'm missing tests for:

Ensuring created_at time is set correctly when creating
Ensuring modified_at time is set correctly when updating
Ensuring created_at filter works properly
Ensuring modified_at filter works properly

Let me know if you need help with any of those.

Thank you!

internal/view/content.html

internal/webserver/handler-api.go

…ated and modified is not in same day

fmartingr · 2024-05-15T09:52:47Z

Forgot to check, but we should probably add indexes in the datetime fields since we are going to rely on those heavily on results.

fmartingr

Great job @Monirzadeh !! This is starting to look real good 👏

I have left some requests around. Most are just style things but there are some concerns around. Please take a look and let me know what you think.

fmartingr · 2024-06-08T16:36:18Z

internal/model/bookmark.go

@@ -14,7 +14,8 @@ type BookmarkDTO struct {
 	Excerpt       string `db:"excerpt"       json:"excerpt"`
 	Author        string `db:"author"        json:"author"`
 	Public        int    `db:"public"        json:"public"`
-	Modified      string `db:"modified"      json:"modified"`
+	Created       string `db:"created_at"    json:"created"`


Can we name this fields CreatedAt / ModifiedAt as well?

json:"created" should be json:"createdAt" or json:"created_at"

For Golang code, CamelCase, for JSON snake_case. Though honestly we could just match everything to the same CamelCase in golang and avoid writting so many json annotations. What do you think?

personalty i am not against none of them. currently we have both of them in json (like imageURL, hasContent or snake case like create_archive and create_ebook)
i am worried about shiori app backward compatibility (if i am not wrong it work with 1.5.5)
as i see there is not specific suggestion in json but usualy use snake_case
@DesarrolloAntonio what do you think?

@DesarrolloAntonio what do you think?

Regarding backward compatibility in the Shiori app, I have seen that there is an option to set one or more alternative names. I have never tried it, but theoretically, it should work.`

@SerializedName("createdAt", alternate = ["created_at"]) val createdAt: String?,

Thanks
So @fmartingr if you like camelCase can go fot that
Beside camelCase or snake_case I think it better to done that in separate PR so later we can refrence that in future issue related to API if anybody ask.
I already use camelCase in this PR

fmartingr · 2024-06-08T16:37:10Z

internal/view/content.html

@@ -38,6 +37,9 @@
 		<div id="content" dir="auto" v-pre>
 			$$.HTML$$
 		</div>
+        <div id="footer">


Can this be a <footer> element rather than a div#footer ?

currently we have footer for two place Version in login page and here.
do you want two diffrent footer style?
.footer and another content-footer?

shiori/internal/view/assets/less/common.less

Lines 8 to 10 in 9aa8332

footer {

color: var(--color);

}

and

#footer { width: 100%; padding: 20px; max-width: 840px; margin-bottom: 16px; background-color: var(--contentBg); border: 1px solid var(--border); display: flex; flex-flow: column; align-items: center; #metadata { display: flex; flex-flow: row wrap; text-align: center; font-size: 16px; color: var(--colorLink); &:nth-child(1) { justify-content: flex-start; } &:nth-child(2) { justify-content: flex-end; } &[v-cloak] { visibility: hidden; } } #title { padding: 8px 0; grid-column-start: 1; grid-column-end: -1; font-size: 36px; font-weight: 700; word-break: break-word; hyphens: none; text-align: center; } #links { display: flex; flex-flow: row wrap; a { padding: 0 4px; color: var(--color); text-decoration: underline; &:hover, &:focus { color: var(--main); } } } }

is 171cea9 what you want?

internal/view/content.html

fmartingr · 2024-06-08T16:43:28Z

internal/view/assets/less/common.less

+		flex-flow: column;
+		align-items: center;
+
+		#metadata {


Avoid using IDs for styling, use classes instead.

is it what you want? 19b9221
i remove some duplicate style and i can review other part and change them to class too. at least in content page.

the metadata color not much readable in light mode (look at the color of added time in light mode)
i test color like normal text (black) but not good looking. i thinking about make this a little darker in light mode. any suggestion?

as you see in light mode it is not readable enough

fmartingr · 2024-06-08T16:46:25Z

internal/database/database_test.go

+func testModifiedTimeUpdate(t *testing.T, db DB) {
+	ctx := context.TODO()


Since this test uses time.Sleep let's run it in parallel to avoid blocking other tests.

Suggested change

func testModifiedTimeUpdate(t *testing.T, db DB) {

ctx := context.TODO()

func testModifiedTimeUpdate(t *testing.T, db DB) {

t.Parallel()

ctx := context.TODO()

i don't see those error in local system that show me that here 9c02e7b for example in golangci-lint

fmartingr · 2024-06-08T16:46:54Z

internal/database/database_test.go

+func testModifiedAndCreateTimeFilter(t *testing.T, db DB) {
+	ctx := context.TODO()


Since this test uses time.Sleep let's run it in parallel to avoid blocking other tests.

Suggested change

func testModifiedAndCreateTimeFilter(t *testing.T, db DB) {

ctx := context.TODO()

func testModifiedAndCreateTimeFilter(t *testing.T, db DB) {

t.Parallel()

ctx := context.TODO()

internal/database/database_test.go

Co-authored-by: Felipe Martin <812088+fmartingr@users.noreply.github.com>

Monirzadeh · 2024-06-12T12:35:46Z

@fmartingr
as you see when i use t.Parallel() when two test run side by side than number and id of item in second test can be diffident so it can give use false negative and affect by each test run. for example this time it failed in Linux but other pass that i think it is not related to the platform
how do you think i should solve that.

fmartingr · 2024-06-26T17:31:40Z

@fmartingr as you see when i use t.Parallel() when two test run side by side than number and id of item in second test can be diffident so it can give use false negative and affect by each test run. for example this time it failed in Linux but other pass that i think it is not related to the platform how do you think i should solve that.

Ok I was thinking that we received a new empty database on each run but right now that's only true for SQLite (not for Postgres/Mysql). Let's remove the t.Parallel for now but please add a TODO comment:

// TODO: Consider using `t.Parallel()` once we have automated database tests spawning databases using testcontainers.

Monirzadeh added 2 commits May 1, 2024 12:44

sqlite migrate script

acb1e1c

create time just when bookmark added and modified update if change ha…

e2aa97a

…ppen

Monirzadeh added 7 commits May 1, 2024 13:13

Merge branch 'master' into add-created-time

7d37ff8

show added and modified time in footer instead of header

55673cf

Merge branch 'add-created-time' of github.com:Monirzadeh/shiori into …

cada6fe

…add-created-time

add bun.lockb that missing

bca2910

add migrate for postgres

428ad58

Merge branch 'master' into add-created-time

3eabf23

add pg support of created time

3083cc3

Monirzadeh added 2 commits May 7, 2024 11:51

Merge branch 'master' into add-created-time

524ef8f

change modifed to modifed_at and create to created_at in sqlite

3d6c031

Monirzadeh commented May 7, 2024

View reviewed changes

Monirzadeh added 2 commits May 7, 2024 13:23

change modifed to modifed_at and create to created_at in postgre

164a9e2

add created_at to mariadb

1f53e5c

Monirzadeh requested a review from fmartingr May 7, 2024 13:09

Monirzadeh mentioned this pull request May 8, 2024

feat: deleted time added to the database #903

Open

Monirzadeh and others added 3 commits May 12, 2024 19:37

Merge branch 'master' into add-created-time

e8ea52e

fix migration file names

c56eb54

Merge branch 'master' into add-created-time

bfc21c2

fmartingr changed the title ~~feat: Created time added to the database and modified time update if bookmark change~~ feat: store created and modified time for bookmarks May 12, 2024

fmartingr requested changes May 12, 2024

View reviewed changes

Monirzadeh added 2 commits May 13, 2024 00:27

better variable name and more clear code for add modified time if cre…

2522d29

…ated and modified is not in same day

Merge branch 'master' into add-created-time

bfe3105

Monirzadeh added 4 commits June 8, 2024 17:44

build new styles.css

d6c02fd

update swagger documents

243f84d

Merge branch 'master' into add-created-time

ffbee63

make styles

bd65ec9

fmartingr requested changes Jun 8, 2024

View reviewed changes

Monirzadeh and others added 16 commits June 9, 2024 19:42

Merge branch 'master' into add-created-time

c99350d

change Created and Modified to CreatedAt and ModifiedAt

5c5a54e

fix missing Modified

218257f

fix typo

9670b48

missing Modified

4df7436

fix typo

5549fe8

make swagger

c17faf4

run tests parallel

9c02e7b

Co-authored-by: Felipe Martin <812088+fmartingr@users.noreply.github.com>

remove t.Parallel()

7567214

remove dayjs dependency and combine two function

e1357ce

better unittest name

535fccc

fix typo

123d003

diffrnt footer style for login and content page

171cea9

use class instead of id

19b9221

back parallel

aec5089

change duplicate url

62c754b

Monirzadeh requested a review from fmartingr June 12, 2024 12:35

Monirzadeh added 3 commits June 26, 2024 21:09

remvoe run Parallel

6af16ee

Merge branch 'master' into add-created-time

d11c0e3

make styles

7db5cd9

fmartingr changed the title ~~feat: store created and modified time for bookmarks~~ feat: store created and modified time separately on database for bookmarks Jun 26, 2024

fmartingr merged commit 4a5564d into go-shiori:master Jun 26, 2024
9 checks passed

Monirzadeh deleted the add-created-time branch June 26, 2024 18:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: store created and modified time separately on database for bookmarks #896

feat: store created and modified time separately on database for bookmarks #896

Monirzadeh commented May 1, 2024 •

edited

Loading

codecov bot commented May 1, 2024 •

edited

Loading

fmartingr commented May 6, 2024

Monirzadeh commented May 6, 2024 •

edited

Loading

fmartingr commented May 6, 2024

Monirzadeh May 7, 2024

Monirzadeh commented May 7, 2024

Monirzadeh commented May 7, 2024 •

edited

Loading

fmartingr left a comment

fmartingr commented May 15, 2024

fmartingr left a comment

fmartingr Jun 8, 2024

Monirzadeh Jun 9, 2024

fmartingr Jun 9, 2024

Monirzadeh Jun 10, 2024 •

edited

Loading

DesarrolloAntonio Jun 10, 2024

Monirzadeh Jun 10, 2024

fmartingr Jun 8, 2024

Monirzadeh Jun 10, 2024 •

edited

Loading

Monirzadeh Jun 10, 2024

fmartingr Jun 8, 2024

Monirzadeh Jun 10, 2024 •

edited

Loading

Monirzadeh Jun 10, 2024 •

edited

Loading

fmartingr Jun 8, 2024

Monirzadeh Jun 10, 2024

fmartingr Jun 8, 2024

Monirzadeh commented Jun 12, 2024

fmartingr commented Jun 26, 2024

		func testModifiedTimeUpdate(t *testing.T, db DB) {
		ctx := context.TODO()

		func testModifiedAndCreateTimeFilter(t *testing.T, db DB) {
		ctx := context.TODO()

feat: store created and modified time separately on database for bookmarks #896

feat: store created and modified time separately on database for bookmarks #896

Conversation

Monirzadeh commented May 1, 2024 • edited Loading

TODO:

codecov bot commented May 1, 2024 • edited Loading

Codecov Report

fmartingr commented May 6, 2024

Monirzadeh commented May 6, 2024 • edited Loading

fmartingr commented May 6, 2024

Choose a reason for hiding this comment

Monirzadeh commented May 7, 2024

Monirzadeh commented May 7, 2024 • edited Loading

fmartingr left a comment

Choose a reason for hiding this comment

fmartingr commented May 15, 2024

fmartingr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Monirzadeh Jun 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Monirzadeh Jun 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Monirzadeh Jun 10, 2024 • edited Loading

Choose a reason for hiding this comment

Monirzadeh Jun 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Monirzadeh commented Jun 12, 2024

fmartingr commented Jun 26, 2024

Monirzadeh commented May 1, 2024 •

edited

Loading

codecov bot commented May 1, 2024 •

edited

Loading

Monirzadeh commented May 6, 2024 •

edited

Loading

Monirzadeh commented May 7, 2024 •

edited

Loading

Monirzadeh Jun 10, 2024 •

edited

Loading

Monirzadeh Jun 10, 2024 •

edited

Loading

Monirzadeh Jun 10, 2024 •

edited

Loading

Monirzadeh Jun 10, 2024 •

edited

Loading