Feat 129 update mentor tags#133
Conversation
yellalena
left a comment
yellalena
left a comment
There was a problem hiding this comment.
great work, here's the first round of review - I'd take another thorough look after the changes are made 😊
| .first(); | ||
| } | ||
|
|
||
| async function createTag(name, trx) { |
There was a problem hiding this comment.
great job preparing our service to use transactions 👏
There was a problem hiding this comment.
Thanks Aliona, but I think I have to remove it for now, since I can't pass the trx from service level for now
There was a problem hiding this comment.
because then i will have to import knex in service level, which I think is not in line with architecture.
am I missing something? :)
There was a problem hiding this comment.
that's not a problem, the problem is nesting repository calls which makes the code a bit harder to navigate and can cause potential import problems.
as we're trying to separate concerns, we agreed to keep all data aggregation on service layer, leaving repositories with operations they're responsible for only.
however, this doesn't mean we can't import knex anywhere else, the opposite even: the idea of transactions is to keep the whole operation atomic, which by definition means the transaction is supposed to be started on the service level and then passed to all repos.
sometimes it is suggested to have a separate layer for transactions, for example a repo registry, but I feel this is going to be a bit of overengineering in our case. but in general the idea is that the transactions are in very rare cases actually initialized inside of database layer.
but that's ok, in general migrating to transactions would be a rather big piece of work and maybe is worth doing it in a separate PR. so if you don't want to have it here, it's fine
3 participants
This PR covers the following: