MWS: fix editing attachment tiddlers #8455

webplusai · 2024-07-30T12:32:48Z

Issue: #8446
Each time the image was broken on the webpage while following the reproduction steps. It was as a result of the attachment_blob value being set to null when the tiddler was saved.

github-actions · 2024-07-30T12:33:01Z

Confirmed: webplusai has already signed the Contributor License Agreement (see contributing.md)

github-actions · 2024-07-30T12:40:29Z

Confirmed: webplusai has already signed the Contributor License Agreement (see contributing.md)

Jermolene

Thank you @webplusai for the speedy response. It's great to see that you've been able to identify a fix so swiftly with unfamiliar code.

I have only conducted a brief review so far as I am away from my desk. I will try it out properly when I am back.

plugins/tiddlywiki/multiwikiserver/modules/store/sql-tiddler-store.js

Jermolene

Thanks @webplusai much appreciated

plugins/tiddlywiki/multiwikiserver/modules/store/sql-tiddler-store.js

Jermolene

Hi @webplusai thank you, this is looking good. My concerns now are around the edge cases.

Have you seen the test framework in $:/plugins/tiddlywiki/multiwikiserver/commands/mws-test-server.js? I think we need to look at introducing some tests for the attachment handling logic. I would hope it could be done relatively easily if we could figure out a convenient way to bring down the attachment size limit so that the tests can work with attachments of 10s of bytes.

plugins/tiddlywiki/multiwikiserver/modules/store/sql-tiddler-store.js

Jermolene

Hi @webplusai this is looking very good. The new tests are very clear and much appreciated.

There are a couple of minor points but otherwise I think this is ready to merge

Jermolene · 2024-08-22T10:15:27Z

.gitignore

@@ -10,3 +10,6 @@ store/
 /playwright-report/
 /playwright/.cache/
 $__StoryList.tid
+/editions/test/test-store/*
+/coverage
+/.nyc_output


I think the last two entries are no longer required.

Jermolene · 2024-08-22T10:16:41Z

editions/test/tiddlers/tests/test-attachment.js

+Tests attachments.
+
+\*/
+var fs = require('fs');


I apologise in advance, but an aspect of our coding style that is unorthodox is that we prefer strings to use double quotes

Jermolene · 2024-08-22T10:30:37Z

editions/test/tiddlers/tests/test-attachment.js

+/*\
+title: test-attachment.js
+type: application/javascript
+tags: [[$:/tags/test-spec]]


These tests should reside within the multiwikiserver plugin, and the tiddlywiki.info for the editions/test edition should be modified to include the multiwikiserver plugin. (This is the approach that the geospatial plugin uses for its tests).

Jermolene · 2024-08-22T10:31:16Z

plugins/tiddlywiki/multiwikiserver/modules/store/attachments.js

 		created: $tw.utils.stringifyDate(new Date()),
 		modified: $tw.utils.stringifyDate(new Date()),
 		contentHash: contentHash,
 		filename: dataFilename,
-		type: options.type
+		type: options.type,


There's an extraneous comma at the end of this line

plugins/tiddlywiki/multiwikiserver/modules/store/attachments.js

Jermolene · 2024-08-22T10:32:33Z

plugins/tiddlywiki/multiwikiserver/modules/store/attachments.js

 	},null,4));
 	return contentHash;
 };

 /*
 Adopts an attachment file into the store
 */
-AttachmentStore.prototype.adoptAttachment = function(incomingFilepath,type,hash) {
+AttachmentStore.prototype.adoptAttachment = function(incomingFilepath,type,hash,canonicalUri) {


I think we should consistently use _canonical_uri as the variable name

Jermolene · 2024-08-22T10:34:35Z

plugins/tiddlywiki/multiwikiserver/modules/store/sql-tiddler-store.js

+
+  let shouldProcessAttachment = tiddlerFields.text && tiddlerFields.text.length <= attachmentSizeLimit;
+
+  if (existing_attachment_blob) {


Another coding style weirdness is that we don't include whitespace between if and the parenthesis.

@pmario may be able to advise on linter settings that can help with TW coding styles

Jermolene

Thank you again for you patience @webplusai. This is excellent quality of work.

There's a couple of minor points below, but otherwise this is ready to merge.

Jermolene · 2024-08-27T13:36:25Z

package-lock.json

-      "version": "7.22.20",
-      "resolved": "https://registry.npmjs.org/@babel/helper-validator-identifier/-/helper-validator-identifier-7.22.20.tgz",
-      "integrity": "sha512-Y4OZ+ytlatR8AI+8KZfKuL5urKp7qey08ha31L8b3BwewJAoJamTzyvxPR/5D+KkdJCGPq/+8TukHBlY10FX9A==",
+      "version": "7.24.7",


I don't think these changes to package-lock.json are needed anymore.

Jermolene · 2024-08-27T13:37:50Z

package.json

-    "test": "node ./tiddlywiki.js ./editions/test --verbose --version --build index && node ./tiddlywiki ./editions/multiwikiserver/ --build load-mws-demo-data --mws-listen --mws-test-server http://127.0.0.1:8080/ --quit",
+    "start": "node ./tiddlywiki.js ./editions/multiwikiserver --mws-load-plugin-bags --build load-mws-demo-data --mws-listen",
+    "build:test-edition": "node ./tiddlywiki.js ./editions/test --verbose --version --build index",
+    "build:multiwikiserver-edition": "node ./tiddlywiki.js ./editions/multiwikiserver/ --build load-mws-demo-data --mws-listen --mws-test-server http://127.0.0.1:8080/ --quit",


It looks like this should perhaps be test:multiwikiserver-edition?

Jermolene · 2024-08-27T13:39:15Z

plugins/tiddlywiki/jasmine/files/jasmine-core/lib/jasmine-core/jasmine.js

@@ -170,7 +170,7 @@ getJasmineRequireObj().base = function(j$, jasmineGlobal) {
   * Default number of milliseconds Jasmine will wait for an asynchronous spec to complete.
   * @name jasmine.DEFAULT_TIMEOUT_INTERVAL
   */
-  j$.DEFAULT_TIMEOUT_INTERVAL = 5000;
+  j$.DEFAULT_TIMEOUT_INTERVAL = 30000;


Files within the plugins/tiddlywiki/jasmine/files directory are taken directly from the Jasmine release. Any changes here will be overwritten the next time Jasmine is upgraded. If the increased timeout is necessary we should explore whether it can be configured in the TiddlyWIki JS wrappers

Jermolene · 2024-08-28T16:13:46Z

Thank you @webplusai for your patience, and congratulations on landing a significant PR.

Jermolene · 2024-08-28T16:21:30Z

Hi @webplusai it turns out that there is a small problem with the tests – they fail if the directory ./editions/test/test-store does not exist. That means that the tests are failing in CI at the moment so it would be great to sort this out as soon as possible.

fix breaking bug in image tiddler attachment

71d67eb

webplusai closed this Jul 30, 2024

webplusai reopened this Jul 30, 2024

Jermolene changed the title ~~fix breaking bug in image tiddler attachment~~ MWS: fix editing attachment tiddlers Jul 30, 2024

Jermolene reviewed Jul 30, 2024

View reviewed changes

webplusai added 2 commits July 31, 2024 07:38

fix comments

a10b574

fix code format

5747373

Jermolene reviewed Jul 31, 2024

View reviewed changes

plugins/tiddlywiki/multiwikiserver/modules/store/sql-tiddler-store.js Outdated Show resolved Hide resolved

webplusai added 2 commits August 1, 2024 07:23

refactor processIncomingTiddler flow

74415bb

remove whitespaces after if statements

f5c9523

Jermolene reviewed Aug 1, 2024

View reviewed changes

plugins/tiddlywiki/multiwikiserver/modules/store/sql-tiddler-store.js Outdated Show resolved Hide resolved

refactor attachment_blob persistence flow

472e93e

Jermolene reviewed Aug 2, 2024

View reviewed changes

webplusai added 5 commits August 7, 2024 09:09

refactor process tiddler to support different attachments

ec8797f

add tests for attachment

169aa74

add more attachement test cases

73aff99

working on adding instanbul for test coverage report

a193247

code coverage report generation

3f02a62

Jermolene mentioned this pull request Aug 21, 2024

[BUG] MWS application/pdf tiddlers can't be renamed. #8541

Closed

remove unnecessary packages

d48d364

Jermolene reviewed Aug 22, 2024

View reviewed changes

fix comments

0d5f687

Jermolene reviewed Aug 27, 2024

View reviewed changes

fix comments

4830f6c

Jermolene merged commit 3287dce into TiddlyWiki:multi-wiki-support Aug 28, 2024
2 of 3 checks passed

Jermolene mentioned this pull request Oct 16, 2024

MultiWikiServer: Editing attachment tiddlers corrupts them #8446

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MWS: fix editing attachment tiddlers #8455

MWS: fix editing attachment tiddlers #8455

webplusai commented Jul 30, 2024 •

edited

Loading

github-actions bot commented Jul 30, 2024

github-actions bot commented Jul 30, 2024

Jermolene left a comment

Jermolene left a comment

Jermolene left a comment

Jermolene left a comment

Jermolene Aug 22, 2024

Jermolene Aug 22, 2024

Jermolene Aug 22, 2024

Jermolene Aug 22, 2024

Jermolene Aug 22, 2024

Jermolene Aug 22, 2024

Jermolene left a comment

Jermolene Aug 27, 2024

Jermolene Aug 27, 2024

Jermolene Aug 27, 2024

Jermolene commented Aug 28, 2024

Jermolene commented Aug 28, 2024


		let shouldProcessAttachment = tiddlerFields.text && tiddlerFields.text.length <= attachmentSizeLimit;

		if (existing_attachment_blob) {

MWS: fix editing attachment tiddlers #8455

MWS: fix editing attachment tiddlers #8455

Conversation

webplusai commented Jul 30, 2024 • edited Loading

github-actions bot commented Jul 30, 2024

github-actions bot commented Jul 30, 2024

Jermolene left a comment

Choose a reason for hiding this comment

Jermolene left a comment

Choose a reason for hiding this comment

Jermolene left a comment

Choose a reason for hiding this comment

Jermolene 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jermolene 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

Jermolene commented Aug 28, 2024

Jermolene commented Aug 28, 2024

webplusai commented Jul 30, 2024 •

edited

Loading