search

hedgedoc / cli

A tiny CLI for HedgeDoc
GNU Affero General Public License v3.0
150 stars 37 forks source link

Make `import-as` display an explicit error message when the given note id conflicts with an existing note #29

Open DougInAMug opened 4 years ago

DougInAMug commented 4 years ago

Hello and thanks for the work thus far!

In our collective, we use the same pad for our weekly meeting. It's quite handy to have the same URL. After the meeting is over, the moderator is tasked with copying the minutes to the website and resetting the minutes template. I thought import-as could be used to help automate this process by uploading the template to the existing pad.

Initial note creation :+1: 

$ codimd import newnote
Found. Redirecting to https://codi.kanthaus.online/zCaOffwOSgGQRRpnaeJM0A

First template import :+1: 

$ codimd import-as zCaOffwOSgGQRRpnaeJM0A exampleTemplate.md
Found. Redirecting to https://codi.kanthaus.online/zCaOffwOSgGQRRpnaeJM0A

Second template import :-1: 

$ codimd import-as zCaOffwOSgGQRRpnaeJM0A exampleTemplate.md
<!DOCTYPE html>
<html lang="en">

<head>
    <meta charset="utf-8">

...

            <h1>500 Internal Error <small>wtf.</small></h1>

I get that this could be an issue with how we set our instance up (and I'll ask our 'sysadmins') but if it could be something with codimd(-cli) I thought I'd let you know.

SISheogorath commented 4 years ago

This almost sounds like a CodiMD server issue, where an import to an existing note is not handled properly. Good catch, I'll check that.

SISheogorath commented 4 years ago

:thinking: After checking the code a bit, I found the error which is raised:

https://github.com/codimd/server/blob/8ce7b285636c02b4215c838640bb748947c8317f/lib/web/note/util.js#L65-L68

But I'm not sure how we would respond better to it. Probably we should answer with a 409 Conflict-HTTP status? Or 405 Method Not Allowed? Because even when it would fit, a 403 would most likely be very misleading.

But yes, this also needs to be handled by the CLI in some way.

DougInAMug commented 4 years ago

Interesting... just a bit out of my grasp to make changes.

Is it intended behavior for codimd import-as to fail if the specified pad already has content, and the issue here is just that the error code is wrong? (In which case, even when it's resolved I'll have to try and whip something else up)

Let me know if I can help somehow... very happy to do user testing ;p

ErikMichelson commented 4 years ago

I've seen your instance is running CodiMD 1.5.0 but the import-as-feature requires a server with version 1.6.0.

At the moment there is no possibility to import contents into an already existing note, therefore the error. But I agree that the error handling should be improved.

DougInAMug commented 4 years ago

Thanks for the info. Although, the import-as did work for me (just the first time) so that's interesting :smile:

If there's no way to import content into existing pads, and the Readme is right about there being no delete command, then I guess I can't do what I wanted right now. I'll see if I can motivate people my end to try and implement something :crossed_fingers:

(I'm happy if you (want me to) close this issue now)

pirate commented 4 years ago

codimd edit <nodeid> <path_to_content.md> is already a feature we intend to add in the future, and I think it should remain distinct from import-as (or at least by a flag like --update). This means that if the import-as destination conflicts with an existing note, it should indeed throw an error, but we can make the error message clearer.

This is for the same reason that CREATE, UPDATE, and UPDATE OR CREATE are distinct in SQL, the expected behavior for one is dangerous for another, and they should likely all exist and remain distinct in codimd-cli as well.

Let's leave this issue open to track fixing the error message, and plan on adding codimd edit and codimd delete as separate features (contributions are welcome for those). For now, note editing is blocked by the lack of a REST API endpoint for updating note content, see https://github.com/codimd/cli/issues/10. If anyone wants to add support for bulk content updating to the server so we don't need to use socket.io from the cli, that would be awesome!