add filtered content from attachment to document

marklogic-community / demo-cat

An application to help SEs find good demos and record bugs and RFEs on them. AngularJS talks to the MarkLogic database via the REST API.

Other

11 stars 17 forks source link

add filtered content from attachment to document #270

Closed janmichaelyu closed 8 years ago

janmichaelyu commented 8 years ago

@grtjn Please review. Let me know if I also need to add the filtered content to be visible in the UI when searching or detail view. #269

grtjn commented 8 years ago

Filtering is a good first step, but ideally i'd expose the result as a memo, which then shows up automatically in details page.

We should try to preserve more formatting though to make that look good. For pdf we have pdf-convert, and for .docx we could disect the zipped xml. Other doc formats will have to do with filtering..

grtjn commented 8 years ago

I think you need to rebase.. :)

janmichaelyu commented 8 years ago

@grtjn Please review.

grtjn commented 8 years ago

Had a quick look just now. I notice you apply fn:string to the result of pdf-convert and doc-filter. Doesn't xdmp:quote make more sense? Memo's should be capable of handling quoted HTML.

Also thinking we may need to prevent people from editing these memos. Can we add a flag to such memos? Something like converted: true?

janmichaelyu commented 8 years ago

Do I also tie the memo to the attachment like if the attachment gets deleted, should I delete the memo?

grtjn commented 8 years ago

I was thinking of disabling editing of memo title, and disallowing deleting such memos, but hadn't considered deletion of attachments themselves yet. Yes, makes sense. You could implement that with a trigger as well perhaps..

janmichaelyu commented 8 years ago

Got it. Please review.

grtjn commented 8 years ago

Looks pretty good at first glance, I'll try to run a test with it this week..

janmichaelyu commented 8 years ago

Please hold off on the merge. Will fix a bug when adding two attachments for conversion at the same time.

janmichaelyu commented 8 years ago

Ok, finished fixing the bug. Please review. Also I noticed some weird page refreshes when the attachments are too big. Is this possible connected to the new trigger?

janmichaelyu commented 8 years ago

Just tested: I disabled the adding of memos and added 3 files, one which is 2Mb. I get 401 Unauthorized in the console and multiple page refreshes until the detail page appears again with the uploaded files.

grtjn commented 8 years ago

The 401's sound suspicious. No worries, I'll test thoroughly before merging.. :-)

grtjn commented 8 years ago

Hmm, you are right, that looks very suspicious. I didn't deploy your changes yet, and am indeed seeing odd network traffic. I'll see if I can make sense out of that..

ryanjdew commented 8 years ago

Added PR #273 that resolves the odd 401 issue.

grtjn commented 8 years ago

Thnx Ryan, I'll see if I can take a renewed look at this PR with your changes piled on top..

grtjn commented 8 years ago

@janmichaelyu I ran a quick test, and it looks good at first glance. A few notes:

switch to pre-commit triggers, that way the new memos will be shown immediately when edit is replaced by the details view of the demo.
I also noticed pdf-convert extracts css, but stores that separately. It is not available in the UI, causing the html extracted from PDF to look a bit funny. Maybe there is a different mode to pdf-convert that looks better, or we may need to adjust what comes out of it.
.doc is not showing as a memo, and are all attachment types filtered? Text from ppt attachments should make demos be found as well. Showing as memo might make less sense, but it should still be found.

janmichaelyu commented 8 years ago

@grtjn I made the changes you asked for. Pre-commit helped a bit but big files still take a while to convert and show up as memos. Let's raise another issue for the PDF extracted memo?

grtjn commented 8 years ago

Yeah, fair enough, this is a good start. Someone else might have interest in jumping in, or maybe I will myself..