Make local file:... links navigate to the relevant file

[aspiers]

Make a link like [[file:foo/bar.org][bar]] into href="foo/bar.org" without target="_blank", so that it opens in the same organice window. This allows more convenient navigation around a network of .org files, and in particular an index.org file which facilitates quick jumping to the most commonly used other .org files.

[aspiers]

I looked at writing tests for this, but got completely confused by the large chunk of test code duplicated between two files. Here is just a small subset of the duplication:

organice/src/lib/parse_org.unit.test.js

Lines 104 to 124 in 591b13c

	describe('HTTP URLs', () => {
	test('Parse a line containing an URL but no /italic/ text before the URL', () => {
	const testOrgFile = readFixture('url');
	const exportedFile = parseAndExportOrgFile(testOrgFile);
	expect(exportedFile).toEqual(testOrgFile);
	});
	});
	describe('www URLs', () => {
	const testOrgFile = readFixture('www_url');
	test('Parse a line containing an URL starting with www', () => {
	const exportedFile = parseAndExportOrgFile(testOrgFile);
	expect(exportedFile).toEqual(testOrgFile);
	});
	test('Parses all valid URLs starting with www', () => {
	const parsedFile = parseOrg(testOrgFile);
	const firstHeader = parsedFile.get('headers').first();
	const parsedUrls = firstHeader.get('description').filter((x) => x.get('type') === 'www-url');
	expect(parsedUrls.size).toEqual(2);
	});
	});

vs.

organice/src/components/OrgFile/OrgFile.unit.test.js

Lines 233 to 255 in 591b13c

	describe('HTTP URLs', () => {
	test('Parse a line containing an URL but no /italic/ text before the URL', () => {
	const testOrgFile = readFixture('url');
	const exportedFile = parseAndExportOrgFile(testOrgFile);
	expect(exportedFile).toEqual(testOrgFile);
	});
	});
	describe('www URLs', () => {
	const testOrgFile = readFixture('www_url');
	test('Parse a line containing an URL starting with www', () => {
	const exportedFile = parseAndExportOrgFile(testOrgFile);
	expect(exportedFile).toEqual(testOrgFile);
	});
	test('Parses all valid URLs starting with www', () => {
	const parsedFile = parseOrg(testOrgFile);
	const firstHeader = parsedFile.get('headers').first();
	const parsedUrls = firstHeader
	.get('description')
	.filter((x) => x.get('type') === 'www-url');
	expect(parsedUrls.size).toEqual(2);
	});
	});

What's going on here?

[schoettl]

I'm confused, too. I remember that I wrote many of these tests. But I certainly did not duplicate this code. As far I remember, I worked mostly in parse_org.unit.test.js.

Is the second one some auto-generated artifact? I can check the commit history later...

[munen]

Good catch on the duplication of tests. Complete duplications can definitively go.

Feel free to pick and chose the correct location depending on the context of the test.

[aspiers]

@munen commented on May 29, 2020 3:43 PM:

Good catch on the duplication of tests. Complete duplications can definitively go.

Feel free to pick and chose the correct location depending on the context of the test.

I don't really know enough about the tests yet to make the call on this 🤔

[munen]

Sweet addition. It works well for me on a first try!

Apart from the duplication of the tests, do you plan to write an integration test for this?

[aspiers]

Duplication fixed in #309. I'll look at writing a test on top of that PR.

[aspiers]

New version pushed with integration test.

[schoettl]

Nice!

How should Organice handle non-org file links? I think I didn't see a test like uri.match(/\.org$/) in the changes...

What does Organice do, if we try to open a non-org file?

[munen]

Sweet, great addition to the test-suite!

Good job, @aspiers! 🙏

@schoettl

What does Organice do, if we try to open a non-org file?

It'll show a message that it cannot parse the file or that there was an error downloading the file. In any case, the user can go back to the original file.

[munen]

@aspiers I've added the attribution to the changelog: 67e1390

Great to see you contributing new features with tests, now! 🙏 🙇

[munen]

@aspiers (cc @schoettl): I deeply apologize, but I have merged this PR too eagerly without proper testing. After merging, I found multiple bugs immediately with using my main Org files (which have file links). I have opened an issue for that: #311

Tbh, the three bugs I found should all have been merge blockers. Of course it's on me, that I didn't test properly. But if we can't fix those soon, I'm inclined to revert the merge. It's very easy to get organice into a broken state with the current implementation. And, on master, we usually try to include only features that work. They may be a subset of what the feature could be, but they should behave consistently. This new feature, unfortunately, doesn't.

I'm afraid the implementation would have to be changed to:

• Parse the Org file with file:// links
• On clicking such a link, create an event
• Then, in a new function that will have to be implemented for all sync back-ends, check if that file can be reached
  • If so, open it (not by redirecting, but through an action/reducer)
  • If not, show a message to the user that this file is not reachable through the sync-backend

Of course, there might be other options to implement this feature. I'm just writing this solution down here, because that's how I envisioned the feature before. Since it's not trivial, I haven't gotten around to building it.