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.

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

Create modeler api tutorial #5

Merged
merged 21 commits into from
Aug 9, 2024
Merged

Conversation

christinaausley
Copy link
Contributor

Drafting up Modeler API tutorial. Docs to follow once this is polished up, but a few questions.

@christinaausley christinaausley self-assigned this May 8, 2024
.env.example Outdated Show resolved Hide resolved
modeler.js Outdated Show resolved Hide resolved
modeler.js Outdated Show resolved Hide resolved
@christinaausley christinaausley requested a review from pepopowitz May 8, 2024 14:54
@christinaausley
Copy link
Contributor Author

@pepopowitz I think this is ready for a first glance, but I have a few loose ends I'm not sure about.

If you'd rather, feel free to pop a half hour on my calendar and we can discuss.

@pepopowitz
Copy link
Collaborator

@christinaausley I'll give this demo a walkthrough locally today, to finalize a review!

@pepopowitz
Copy link
Collaborator

Note that this tutorial will need to be updated based on #7, as in camunda/camunda-docs#3708.

@christinaausley
Copy link
Contributor Author

@pepopowitz I've cleaned this up to mirror the updated auth -- let me know if you want to review this async, or jump on a call 👍

modeler.js Outdated Show resolved Hide resolved
@pepopowitz
Copy link
Collaborator

@christinaausley I wasn't sure where to place this feedback relative to the changed code, so I'm making a general comment.

I propose that we implement at least one API call different than we currently are.

We're implementing two methods in this tutorial: one to retrieve details of a specific file, one to delete a specific file. The challenge for the user is that they'll need to go into the Modeler to find a file suitable for deletion, and capture only its ID from the URL. The URLs for files in Modeler don't make it easy to capture the ID -- for example, the ID for https://modeler.cloud.camunda.io/process-applications/8f413a9a-ba96-4908-8c03-7887ebf161f7--basic/ is 8f413a9a-ba96-4908-8c03-7887ebf161f7 (note that the app appends the name of the file to the ID). This then becomes our problem, of having to explain to them how to capture that ID from the URL. Opportunities for things to go wrong, things that aren't really the main focus of the tutorial.

I have two proposals -- one more involved than the other.

  1. Instead of the "retrieve" method, we provide a "list" method, like we've done in the other API tutorials. This would allow them to list out all IDs from their new CLI, and to capture one there more easily than in the Modeler URL.
  2. Instead of the "files" resource, we implement the "projects" resource. The "projects" resource enables us to create a test item more simply than the "files" resource. Check out the specs -- a new project requires only a name property, compared to a file which requires a valid projectId, folderId, and contents. That would allow us to implement all the methods we did in the Administration API tutorial (list, add, view, delete) -- which would allow for the user to follow the tutorial without affecting existing files, which they may or may not feel comfortable deleting.

What do you think? I'm open to other suggestions too, including iterative approaches like "let's do these methods and then come back and do one of the above suggestions." I'm also happy to contribute any changes or help if you are annoyed by me kicking this review a month down the road, and then suggesting such drastic changes.

@christinaausley
Copy link
Contributor Author

  1. Instead of the "retrieve" method, we provide a "list" method, like we've done in the other API tutorials. This would allow them to list out all IDs from their new CLI, and to capture one there more easily than in the Modeler URL.

@pepopowitz This is an easy fix! I can probably do this today. But, do you have a preference on how we handle this? Could always open a backlog issue for iterations on these tutorials.

@christinaausley
Copy link
Contributor Author

@pepopowitz I ended up using POST for the first API call, so the user creates a semi-meaningless file, the output reveals the name and ID, and then the user deletes it with this ID. WDYT?

@pepopowitz
Copy link
Collaborator

@pepopowitz I ended up using POST for the first API call, so the user creates a semi-meaningless file, the output reveals the name and ID, and then the user deletes it with this ID. WDYT?

Did that work for you against the actual API? When I was playing with the API in Insomnia, I was unable to create a file that didn't have contents.

@pepopowitz
Copy link
Collaborator

  1. Instead of the "retrieve" method, we provide a "list" method, like we've done in the other API tutorials. This would allow them to list out all IDs from their new CLI, and to capture one there more easily than in the Modeler URL.

@pepopowitz This is an easy fix! I can probably do this today. But, do you have a preference on how we handle this? Could always open a backlog issue for iterations on these tutorials.

I don't have a preference. In case you'd like to handle it with a follow-up, I'll approve this one as-is, once I confirm that posting a file without contents functions against the real API.

@pepopowitz
Copy link
Collaborator

@pepopowitz I ended up using POST for the first API call, so the user creates a semi-meaningless file, the output reveals the name and ID, and then the user deletes it with this ID. WDYT?

Did that work for you against the actual API? When I was playing with the API in Insomnia, I was unable to create a file that didn't have contents.

No, this API call does not work.

See my previous comment about the File resource in the modeler API. Creating a file has several requirements, all of which will make it more work for us compared to a resource like Folder. Most of the requirements are captured in the swagger description, though not all of them (like there need to be valid file contents).

If you'd like to treat the swapping of resources from Files to Folders iteratively, then I recommend reverting the changes to create the file.

@christinaausley
Copy link
Contributor Author

No, this API call does not work. See my previous comment about the File resource in the modeler API.

AH! I understand now. I'll just adjust the calls to the Projects 👍

@christinaausley
Copy link
Contributor Author

This is ready for review @pepopowitz, but if you don't get to it today, I can have @conceptualshark and/or @akeller have a look while you're away 😄

Upon approval, I'll update the documentation. Thanks for your help!

Copy link
Collaborator

@pepopowitz pepopowitz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IGNORE this review, the suggestions somehow ended up on the incorrect lines.

modeler.js Outdated Show resolved Hide resolved
modeler.js Outdated Show resolved Hide resolved
modeler.js Outdated Show resolved Hide resolved
modeler.js Show resolved Hide resolved
modeler.js Outdated Show resolved Hide resolved
modeler.js Outdated Show resolved Hide resolved
modeler.js Outdated Show resolved Hide resolved
@pepopowitz
Copy link
Collaborator

pepopowitz commented Jun 21, 2024

Oh man, I tried to make all my suggestions from within VS code, and it botched all the line numbers. I'm going to dismiss my previous review after re-submitting through the UI so that it's not suggesting misaligned changes 😭

@pepopowitz pepopowitz dismissed their stale review June 21, 2024 18:34

botched line numbers

Copy link
Collaborator

@pepopowitz pepopowitz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nicely done, again! I left a few blocking requests re: variable names; anything non-blocking is specified as such.

Thanks for your patience with my requests to refine this!

modeler.js Outdated Show resolved Hide resolved
modeler.js Outdated Show resolved Hide resolved
modeler.js Outdated Show resolved Hide resolved
modeler.js Outdated Show resolved Hide resolved
modeler.js Outdated Show resolved Hide resolved
modeler.js Outdated Show resolved Hide resolved
modeler.js Outdated Show resolved Hide resolved
christinaausley and others added 6 commits June 24, 2024 12:05
Co-authored-by: Steven Hicks <[email protected]>
Co-authored-by: Steven Hicks <[email protected]>
Co-authored-by: Steven Hicks <[email protected]>
Co-authored-by: Steven Hicks <[email protected]>
Co-authored-by: Steven Hicks <[email protected]>
Co-authored-by: Steven Hicks <[email protected]>
.env.example Outdated Show resolved Hide resolved
modeler.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@pepopowitz pepopowitz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While playing with the code in this tutorial, I think I settled on a more ideal approach. I've written the code for it in #9.

Basically, it encapsulates the creation of a project and the addition of a collaborator into one action. It will take a little more explanation in the docs, to tell them why we're adding a second API call to this action and to walk them through the code.

But I think this aligns with what a user of our APIs would likely do in this situation. It doesn't appear that there's any endpoint in the Modeler API that can be used without the pre-existence of some other entity. In this case, the pre-existence required is the own user's account, which I think we can assume.

@christinaausley
Copy link
Contributor Author

But I think this aligns with what a user of our APIs would likely do in this situation. It doesn't appear that there's any endpoint in the Modeler API that can be used without the pre-existence of some other entity. In this case, the pre-existence required is the own user's account, which I think we can assume.

@pepopowitz Thank you for this! I'll update the documentation once this PR is finalized, and Slack you if I have any questions about how you've set it up.

.env.example Outdated Show resolved Hide resolved
@christinaausley christinaausley marked this pull request as ready for review August 7, 2024 15:10
Copy link
Collaborator

@pepopowitz pepopowitz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💪 💪 💪 💪 💪 💪 💪

@christinaausley christinaausley merged commit 8a52b39 into main Aug 9, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants