Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2234 +/- ##
==========================================
+ Coverage 46.17% 46.22% +0.05%
==========================================
Files 1040 1041 +1
Lines 17160 17178 +18
Branches 3133 3129 -4
==========================================
+ Hits 7923 7941 +18
Misses 9237 9237 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
graduta
changed the title
| | `Mattermost` | Mattermost channels | `Food`, `Bookkeeping updates` | | `id` | Update | | ||
| | `email` | Email groups | `food@cern.ch`, `Bookkeeping-updates@cern.ch` | | `id` | Update | | ||
| | `last_edited_name` | Name of the person who last edited the email/mattermost fields | `Anonymous`, `Jan Janssen` | When email/mattermost is edited | `id` | Update | | ||
| | `last_edited_by_user_id` | Id (in `users` table) of the user who last edited the tag | `1`, `2` | When the tag is edited | `id` | Update | |
There was a problem hiding this comment.
'When the tag is edited' reads as if the field is only set when the tag is updated. After this change it's also set when the tag is created. Do you think it could be more specific, or is it clear enough?
| text, | ||
| description, | ||
| lastEditedName: last_edited_name, | ||
| lastEditedBy: lastEditedBy ? this.userAdapter.toNameOnly(lastEditedBy) : null, |
There was a problem hiding this comment.
RunAdapter, LogAdapter and LogService all call tagAdapter.toEntity without the association, so their lastEditedBy is always null. I think we should either include the association or omit the field, what do you think?
| return TagRepository.insert(tagAdapter.toDatabase(body)); | ||
| }); | ||
|
|
||
| return tag ? tagAdapter.toEntity(tag) : null; |
There was a problem hiding this comment.
CreateTagUseCase returns lastEditedBy as null in the create response. UpdateTagUseCase solves this by re-fetching the tag.
Could it be worth avoiding confusion/inconsistency to do the same?
|
|
||
| expect(result.lastEditedBy).to.deep.equal({ name: 'Jan Jansen' }); | ||
|
|
||
| await TagRepository.removeAll(new QueryBuilder().where('id').is(createdTag.id)); |
There was a problem hiding this comment.
In both new tests, if the assertion fails, the removeAll never gets run and the inserted tag remains in the DB. Could the cleanup go in a finally or an afterEach?
isaachilly
left a comment
isaachilly
left a comment
There was a problem hiding this comment.
There are a couple of API changes here: tag creation requiring a session user and lastEditedName being replaced by lastEditedBy. Worth double-checking if you haven't already that these aren't critically relied upon, esepcially in the emails by tags system. Should be clearly mentioned in release notes anyways.
I ran the migration against a local prod dump and every name mapped to a user. Since unmapped names will get lost, it might be worth checking on actual prod for peace of mind that this is also the case.
Other than that and the inline comments, looks good to me.
This branch has not been deployed
2 participants
I have a JIRA ticket
Notable changes for users:
Notable changes for developers:
Changes made to the database: