Delete Comment is not working expected

[surapuramakhil]

Describe the bug

I have made an test comment from my account akhil_test. I have tagged another acount akhil in the comment.

in my akhil_test account (person who wrote the comment) has deleted the comment. The comment is only deleted for him but his comment still exists in akhil account. and in public internet.

Following screenshots are taken at same time. Testing on https://meta.answer.dev/ place where its tested : https://meta.answer.dev/questions/D1eI2/?commentId=10070000000010357

From akhil_test account

public internet (No user loggedIn)

From account Akhil

To Reproduce

Steps to reproduce the behavior:

1. Go to '...'
2. Click on '....'
3. Scroll down to '....'
4. See error

Expected behavior

A clear and concise description of what you expected to happen.

Screenshots

If applicable, add screenshots or video to help explain your problem.

Platform

• Device: [e.g. Desktop, Mobile]
• OS: [e.g. macOS]
• Browser and version: [e.g. Chrome, Safari]
• Version: [e.g. v1.2.0]

[hgaol]

have you refreshed the page?

[surapuramakhil]

yes multiple times. Its still there. Just now I checked.

[hgaol]

I found that if visiting your link which pointed to the comment (https://meta.answer.dev/questions/D1eI2/?commentId=10070000000010357), the comment will stay there. If I visit this question directly (https://meta.answer.dev/questions/D1eI2), it disappeared. Not sure if it's by design.

[zahash]

I made a PR to fix this issue. https://github.com/apache/incubator-answer/pull/799 can one of the maintainers please look into this?

[zahash]

@LinkinStars

[LinkinStars]

@surapuramakhil Thanks for the feedback. Anyway, the deleted content should not be visible to users. @zahash Thanks for the contribution, PR has been merged

[surapuramakhil]

Thank you. @LinkinStars @zahash can you consider adding e2e test case for this.

Case 1: you are able to see comment with the link if it's not deleted. Case 2: you shouldn't be able to see the comment if it is deleted.

[zahash]

oh didn't realize this issue is still open. sure i will add the two test cases.

[zahash]

@fenbox how do i fork the dev branch?

[LinkinStars]

oh didn't realize this issue is still open. sure i will add the two test cases.

@zahash Don't worry, your code has been merged in dev. If you want to add test cases, you can checkout from dev branch.

He just made a mark that the issue is done and to mark that you fixed it.

[zahash]

@LinkinStars

how do i run tests? i get this error when i make test

try to create database directory /
[xorm] [info]  2024/03/21 17:25:05.478650 PING DATABASE sqlite
panic: connection to database failed: unable to open database file: out of memory (14)

goroutine 1 [running]:
github.com/apache/incubator-answer/internal/repo/repo_test_test.TestMain(0xc001209cc0)
        /home/zahash/source/incubator-answer/internal/repo/repo_test/repo_main_test.go:88 +0x255
main.main()
        _testmain.go:165 +0x195
FAIL    github.com/apache/incubator-answer/internal/repo/repo_test      0.037s
FAIL
make: *** [Makefile:32: test] Error 1

btw this is what i do for launching the project

make ui
make build
sudo ./answer run --data-path ./data/

[surapuramakhil]

Are you out of memory? Probably, you can use CI/CD of GitHub for executing test cases. They offer unlimited for open source projects. you can trigger them by raising MR. provided this repo this configured for running test suite.

[zahash]

its not because of memory. for some reason the project is trying to create a temporary database at root. so obviously it doesn't have the permissions

/tmpanswer-test-data.db

func TestMain(t *testing.M) {
	dbSetting, ok := dbSettingMapping[os.Getenv("TEST_DB_DRIVER")]
	if !ok {
		// Use sqlite3 to test.
		dbSetting = dbSettingMapping[string(schemas.SQLITE)]      // <<<<<<<<<<<<< HERE
	}
	if dbSetting.Driver == string(schemas.SQLITE) {
		os.RemoveAll(dbSetting.Connection)
	}

	defer func() {
		if tearDown != nil {
			tearDown()
		}
	}()
	if err := initTestDataSource(dbSetting); err != nil {
		panic(err)
	}
	log.Info("init test database successfully")

	if ret := t.Run(); ret != 0 {
		panic(ret)
	}
}

[zahash]

@LinkinStars i found the bug.

there should be a slash between temp dir and the db name

	sqlite3DBSetting = TestDBSetting{
		Driver:     string(schemas.SQLITE),
		Connection: os.TempDir() + "answer-test-data.db",   // <<<<<< HERE
	}

right now, instead of creating a database at /tmp/answer-test-data.db it instead tries to create /tmpanswer-test-data.db

[zahash]

@LinkinStars created a pull to fix that issue #871

[surapuramakhil]

@zahash these test cases were created for container environment I guess. It's not a bug, it's actually enhancement

[zahash]

@zahash these test cases were created for container environment. It's not a bug, it's actually enhancement

no i don't think so. if it was created for container environment, why not just do Connection: "answer-test-data.db" besides, docker returns /tmp for os.TempDir() (assuming the image is based on some linux distro)

they clearly tried to put it in temp dir but made a mistake joining the paths

[surapuramakhil]

Possible hacks :joy:. No code is perfect.

[zahash]

@LinkinStars @fenbox

here is the PR that adds tests to make sure deleted comments cannot be retrieved #872 and another PR to fix the filepath bug #871 (context: https://github.com/apache/incubator-answer/issues/796#issuecomment-2012102102)

[zahash]

@surapuramakhil

eh, mistakes like this happen from time to time.

but it is a little strange that Go thinks filepaths and filenames are utf8 strings. because in most operating systems they can be arbitrary byte sequences separated by slashes. (because most operating systems predate UTF-8)

[zahash]

@LinkinStars @surapuramakhil @fenbox can this issue be closed now that all the PRs have been merged? #872 #871

[LinkinStars]

@LinkinStars @surapuramakhil @fenbox can this issue be closed now that all the PRs have been merged? #872 #871

@zahash We usually close the issue after releasing a new version.