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

fix(hierarchy): should update :block/namespace while renaming page #8266

Merged
merged 8 commits into from Jan 11, 2023

Conversation

situ2001
Copy link
Collaborator

@situ2001 situ2001 commented Jan 8, 2023

close #8253

Because renaming the namespace page is happened from top to bottom of the hierarchy, it is very straightforward to add a function that updates :block/namespace of a block to its parent block or removes :block/namespace of a block. If there is no parent block, the code invokes create! to create it, for example, renaming aa to aa/bb/cc/dd will automatically create aa, aa/bb, aa/bb/cc if not exist.

BTW, there is currently no description for the namespace in logseq doc so I can't get full or correct understanding of how renaming a namespace page work.

The behavior of renaming the namespace block is summarized that changing the name of a namespace page, the name of the page, and its sub-namespace page will be changed.

Imagine there are some pages.

1
1/2
1/2/4
1/2/5
1/3
1/3/6
1/3/7

Changing from 1/3 -> 1/4 will cause

1/3 -> 1/4
1/3/6 -> 1/4/6 ;; namespace is changed from 1/3 to 1/4
1/3/7 -> 1/4/7 ;; namespace is changed from 1/3 to 1/4

Other situation

  • Changing from aa/bb -> aabb will remove :block/namespace of this page.
  • Changing from tony -> cat/tony will add :block/namespace of this page to cat. If cat does not exist, cat page will be created.

I have no idea where to write the test for this fix(or maybe a feature?) because I didn't find the test that simulates an env that has DataScript DB.

This is my first time contributing to the codes that involve to DataScript DB transactions. Please review it carefully. Thanks.

@github-actions github-actions bot added the fix label Jan 8, 2023
@Bad3r Bad3r added the looking-for-review PRs that requires attention label Jan 9, 2023
@andelf andelf requested review from andelf and cnrpman January 9, 2023 05:24
Copy link
Collaborator

@andelf andelf left a comment

Choose a reason for hiding this comment

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

LGTM

src/main/frontend/handler/page.cljs Outdated Show resolved Hide resolved
@cnrpman
Copy link
Collaborator

cnrpman commented Jan 10, 2023

BTW, there is currently no description for the namespace in logseq doc so I can't get full or correct understanding of how renaming a namespace page work.

Yes, the document is missing 👀

I have no idea where to write the test for this fix(or maybe a feature?) because I didn't find the test that simulates an env that has DataScript DB.

We have test for db model but fine to leave it to future

(ns frontend.db.model-test

@Bad3r Bad3r added this to the 0.8.16 milestone Jan 10, 2023
new-namespace? (text/namespace-page? new-name)
update-namespace! (fn [] (let [namespace (first (gp-util/split-last "/" new-name))]
(when namespace
(create! namespace {:redirect? false}) ;; create parent page if not exist, creation of namespace ref is handled in `create!`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Smart :)

Copy link
Collaborator

@cnrpman cnrpman left a comment

Choose a reason for hiding this comment

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

LGTM.
Need @tiensonqin to have a confirm as it's about DB ops

@andelf andelf merged commit c228863 into logseq:master Jan 11, 2023
@logseq-cldwalker
Copy link
Collaborator

BTW, there is currently no description for the namespace in logseq doc so I can't get full or correct understanding of how renaming a namespace page work.
Good point! Hopefully we address this soon

I have no idea where to write the test for this fix(or maybe a feature?) because I didn't find the test that simulates an env that has DataScript DB.

@situ2001 Tests that are using the test-helper load-test-files or test-db, exercise the datascript db. The nice thing about load-test-files is that you can write tests from the user perspective e.g. the files and the exact content that they would have. If you'd like to write tests for this per #8298, happy to review it and give guidance as needed

cnrpman added a commit to cnrpman/logseq that referenced this pull request Jan 12, 2023
…ogseq#8266)

* fix: should update `block:namespace` while renaming
* fix: check logic in `rename-update-namespace!`

Co-authored-by: Bad3r <bad3r@protonmail.com>
Co-authored-by: Junyi Du <junyidu.cn@gmail.com>
sallto pushed a commit to sallto/logseq that referenced this pull request Jan 18, 2023
…ogseq#8266)

* fix: should update `block:namespace` while renaming
* fix: check logic in `rename-update-namespace!`

Co-authored-by: Bad3r <bad3r@protonmail.com>
Co-authored-by: Junyi Du <junyidu.cn@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix looking-for-review PRs that requires attention
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Hierarchy] :block/namespace of a page doesn't updated after renaming the page
5 participants