-
Notifications
You must be signed in to change notification settings - Fork 5
feat: duplicate and delete templates, add toggle for raw issuance #242
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
base: beta
Are you sure you want to change the base?
Conversation
…ple `Cargo.toml` files
| DuplicateTemplate { | ||
| duplicate_from: String, | ||
| new_template_id: String, | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one we don't really need anymore right since you now added duplicate_from to CreateTemplate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could also do it the other way: not introduce duplicate_from to CreateTemplate and have a separate DuplicateTemplate command, which implicitly just copies all fields. Otherwise, you'd have to supply all other fields as well when calling CreateTemplate (same display, same credential_format, etc.).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that what you suggest would indeed be the right DDD way of doing this but atm technically that would be still a bit tricky to do. When the Template::handle function receives the command:
DuplicateTemplate {
duplicate_from: String,
new_template_id: String,
}then it has no way of accessing the field data from duplicate_from. What we would need then is define a LibraryService that acts as a Domain Service. This Domain Service then should contain access to the LibraryState and it should have a method that simply accepts a Template ID, and returns the TemplateView which can be used in the Template::handle function to do the duplication. However, the problem here is that if we want to do this this will require some refactor related to agent_store (which I have started btw but not completed yet).
But once we have that then I think it's a good to idea to implement the LibraryService so what we can do what you suggested. So, TODO comment for now? 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that this would be a bit tricky for the moment. I added a TODO for now.
agent_library/src/template/event.rs
Outdated
| }, | ||
| TemplateDuplicated { | ||
| duplicate_from: String, | ||
| new_template_id: String, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to https://github.com/impierce/ssi-agent/pull/242/files#r2589651162, we don't need this one right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| self.modified_at.replace(modified_at.clone()); | ||
| } | ||
| TemplateDeleted { template_id: _ } => { | ||
| self.status = Status::Deleted; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the code you added to the apply function for this Event in aggregate.rs:
*self = Self::default();
self.template_id = template_id;
self.status = Status::Deleted;Here all the values should also be 'wiped'.
| #[derive(Deserialize, Serialize, Default)] | ||
| #[serde(default, rename_all = "camelCase")] | ||
| pub struct PostTemplatesEndpointRequest { | ||
| pub id: Option<String>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you branched off from my previous PR before it got merged I removed the id field from this request struct so you can also remove it again.
| // Fetch original template data. | ||
| let original_template = query_handler(&old_template_id, &state.query.template) | ||
| .await? | ||
| .ok_or_else(|| ApiError::new(StatusCode::NOT_FOUND))?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Returning a 404 NOT FOUND status code would communicate to the client that the endpoint itself does not exist, which is not true. I think it is better to use 422 Unprocessable Content here and include a Problem Details. You can follow this example in credentials.rs (which also uses NOT_FOUND which is wrong so please add a TODO comment there 😅 ):
ApiError::builder(StatusCode::NOT_FOUND)
.title("No Credential Configuration Found")
.type_url(type_url("issuance#no-credential-configuration-found"))
.message(format!(
"No Credential Configuration found with id: `{credential_configuration_id}`"
))
.finish()What this means as well is that you will need to add the description of the error as well to the docs folder. You will need to add a library.md file and include the description (you can take a look at the Problem Details descriptions we already have in the other .md files for inspiration).
| display: original_template.display.clone(), | ||
| credential_format: original_template.credential_format.clone(), | ||
| creator: original_template.creator.clone(), | ||
| holder_type: original_template.holder_type.clone(), | ||
| tags: original_template.tags.clone(), | ||
| status: Status::Draft, | ||
| visibility: original_template.visibility.clone(), | ||
| description: original_template.description.clone(), | ||
| r#type: original_template.r#type.clone(), | ||
| schema: original_template.schema.clone(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I am not mistaken you can remove all the .clone() occurrences here.
| " pm.collectionVariables.set(\"template_id\", response.id);", | ||
| " console.log(\"Saved template_id: \" + response.id);", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here as well as in the Request Body for "Duplicate Template" you use template_id but for the other endpoints we use TEMPLATE_ID. Is that deliberate or can we use TEMPLATE_ID for everything?
| { | ||
| "key": "template_id", | ||
| "value": "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we re-use TEMPLATE_ID instead of adding this one?
|
|
||
| let create_command = TemplateCommand::CreateTemplate { | ||
| template_id: new_template_id.clone(), | ||
| duplicate_from: Some(old_template_id), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although I called it duplicate_from in issue #202, I think that name it isn't fitting too well (apart from that it's grammatically wrong as well -- should be "duplicate of").
When you duplicate a template, then adapt it and change it quite a bit over time, something such as original_template is probably more fitting. This implies a duplicate, but more elegantly.
| pub struct Template { | ||
| #[serde(rename = "id")] | ||
| pub template_id: String, | ||
| pub duplicate_from: Option<String>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…sed Duplicate command and event.
Description of change
Adds a toggle for raw issuance, overriding the need to use a template and allowing manual json input. This can be toggled through the config under the
skip_template_enforcementbool which defaults to false.Further, the commands and corresponding endpoints for DuplicateTemplate and DeleteTemplate have also been added, allowing duplication of existing templates which results in a template with a new ID and the word "Copy" appended to the end of the template's title.
To duplicate a template you can now make a
POSTrequest tov0/templates/duplicate-templatewith the body:To delete a template you can simply make a
POSTrequest to `v0/template/delete-template with the body:Additionally, the previous
PATCHrequest has been now replaced by aPOSTrequest tov0/templates/update-templateto update elements of a template.Our postman collection and OpenAPI documentation have been updated accordingly.
Links to any relevant issues
Addresses parts of Issue #202
How the change has been tested
Describe the tests that you ran to verify your changes.
All existing tests pass. Functionality has also been tested through the postman collection.
Definition of Done checklist
Add an
xto the boxes that are relevant to your changes.