Conversation
…engine with placeholder replacement
…nts and study documents
…sing placeholder names in error
dennis-zyska
left a comment
dennis-zyska
left a comment
There was a problem hiding this comment.
I reviewed the first half of the code, but I have an appointment now, so I will submit the review and come back to it later.
backend/db/migrations/20251227183831-create-template-placeholder-mapping.js
Outdated
Show resolved
Hide resolved
| const emailContent = await getEmailContent( | ||
| "email.template.assignment", | ||
| "CARE - New Review Assignment", | ||
| `Hello, |
There was a problem hiding this comment.
Those default messages doesn't look good in the code, maybe we can create file on the disk that were we read from in case there is not template defined. Also for the other default emails like in the login, that way at least the code looks better.
| } | ||
|
|
||
| const emailContent = await getEmailContent( | ||
| "email.template.studyClosed", |
There was a problem hiding this comment.
Move to file as mentioned in a previous comment
| const emailContent = await getEmailContent( | ||
| "email.template.sessionStart", | ||
| "CARE - Review Session Started", | ||
| `Hello, |
There was a problem hiding this comment.
Move content to disk file
| const emailContent = await getEmailContent( | ||
| "email.template.sessionFinish", | ||
| "CARE - Review Session Completed", | ||
| `Hello, |
There was a problem hiding this comment.
move content to disk file
| transaction: options.transaction, | ||
| }); | ||
|
|
||
| return { success: true }; |
There was a problem hiding this comment.
We don't need to return this, it is already done by the createSocket class if we don't throw an error. You get this with the callback in the frontend (see other socket requests)
| * @param {Object} options.transaction | ||
| * @returns {Promise<Object>} | ||
| */ | ||
| async updateTemplate(data, options) { |
There was a problem hiding this comment.
This function doesn't have any special requirements that can't be handled via a beforeUpdate hook or is not already implemented in our updateData functionality (https://github.com/UKPLab/CARE/blob/main/backend/webserver/sockets/app.js#L64) right? So we should add no functions that are not absolutely necessary to reduce code and maintainability. Please also check the other functions in this class.
| * @param {Object} options.transaction | ||
| * @returns {Promise<Array<string>>} | ||
| */ | ||
| async getLanguages(data, options) { |
There was a problem hiding this comment.
if we retrieve all templates in the frontend, aren't there also other languages retrieved or only the language that is currently selected? because otherwise we already have the languages in the frontend right?
| * @param {Object} options.transaction | ||
| * @returns {Promise<Object>} | ||
| */ | ||
| async addLanguageContent(data, options) { |
There was a problem hiding this comment.
Isn't it just addContent and language is just another variable that is defined in addition?
| }, | ||
| onUpdate: 'CASCADE', | ||
| }, | ||
| published: { |
There was a problem hiding this comment.
Is there a reason why it is published and not public?
| }); | ||
| }, | ||
|
|
||
| filterDefaultLanguageOptions(languagesWithContent) { |
There was a problem hiding this comment.
can we just use a computed for that method?
| : [...SUPPORTED_LANGUAGES]; | ||
| }, | ||
|
|
||
| filterTypeOptionsInStore() { |
There was a problem hiding this comment.
also here, is a computed possible?
| payload.content = { ops: [{ insert: "\n" }] }; | ||
| } | ||
|
|
||
| const eventName = isEdit ? "templateUpdate" : "templateAdd"; |
There was a problem hiding this comment.
see also my other comment in the backend socket, why not using appDataUpdate?
There was a problem hiding this comment.
The file name is misleading, maybe just Placeholder , also for later generalisation
There was a problem hiding this comment.
What I was a bit confused, you basically created another Editor here, but then integraded it in the original Editor? I think there are two ways, either you make the selection one higher level in the component Template.vue where you used the Editor or even better you integrate it well in the existing editor to reduce duplicate code. I'm find with the first solution for now to get it merged, but please have a look in the Editor.vue component and check what changes are really necessary or which can be moved to Template.vue
3 participants
No description provided.