Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Open
DougInAMug opened this issue Mar 4, 2020 · 6 comments
Open

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

DougInAMug opened this issue Mar 4, 2020 · 6 comments

Comments

@DougInAMug
Copy link

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 👍

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

First template import 👍

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

Second template import 👎

$ 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
Copy link
Collaborator

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
Copy link
Collaborator

SISheogorath commented Mar 4, 2020 •
edited
Loading

🤔 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
Copy link
Author

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
Copy link
Member

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
Copy link
Author

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

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 🤞

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

@pirate
Copy link
Member

pirate commented Mar 24, 2020 •
edited
Loading

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 #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!

@pirate pirate changed the title codimd import-as results in 500 error after first usage Make import-as display an explicit error message when the given note id conflicts with an existing note Mar 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@pirate @SISheogorath @DougInAMug @ErikMichelson