Fix unread sync #369
Fix unread sync #369
Conversation
| const { metadataKeys, unread } = params; | ||
| let badgeCounter = 0; | ||
| for (const metadataKey of metadataKeys) { | ||
| const [email] = await getEmailByKey(metadataKey); |
There was a problem hiding this comment.
running a query inside a loop is a bad practice.
There was a problem hiding this comment.
Is promise.all ok?
There was a problem hiding this comment.
No. load data in batches.
| return db.transaction(async trx => { | ||
| await deleteEmailsByIds([id], trx); | ||
| await deleteEmailContactByEmailId(id, trx); | ||
| await deleteEmailLabelsByEmailId(id, trx); |
There was a problem hiding this comment.
if you use cascade triggers there is no need for you to delete relations of emails you already deleted.
There was a problem hiding this comment.
This is pending in other pull request, to handle this. I just change the position
electron_app/src/DBManager.js
Outdated
| return db | ||
| .select('*') | ||
| .table(Table.EMAIL) | ||
| .whereIn('id', emailIds); |
There was a problem hiding this comment.
wouldn't it be better to use a join instead of two queries?
| const emailsId = emailLabels.map(item => item.emailId); | ||
| if (emailsId.length) { | ||
| await deleteEmailLabel({ emailsId, labelId: id }, trx); | ||
| } |
There was a problem hiding this comment.
again, triggers should handle this.
There was a problem hiding this comment.
This is pending in other pull request, to handle this. I just change the position
| for (const email of emails) { | ||
| await updateEmail({ | ||
| key: email.key, | ||
| unread: !!unread |
There was a problem hiding this comment.
update should also work in batches.
|
|
||
| it('should update email: unread by keys', async () => { | ||
| const keys = ['keyC', 'keyId']; | ||
| await DBManager.updateEmail({ |
There was a problem hiding this comment.
I'm not sure about this. You are updating emails that are used by other tests, this could affect them.
There was a problem hiding this comment.
This query has a test too and I add other test additionally! All passes ok
| `${Table.EMAIL}.id`, | ||
| `${Table.EMAIL_LABEL}.emailId` | ||
| ) | ||
| .whereIn(`${Table.EMAIL_LABEL}.labelId`, labelIds); |
There was a problem hiding this comment.
Yes! The test is ok!
| .whereIn('id', emailIds); | ||
| }; | ||
|
|
||
| const updateEmail = ({ |
There was a problem hiding this comment.
this method is so complex, it does many different things depending on the input parameters. I suggest you split it into smaller, simpler functions that do one thing.
There was a problem hiding this comment.
This is a simple join. And it was tested
There was a problem hiding this comment.
separate: updateEmail and updateEmails ?
There was a problem hiding this comment.
That might be an improvement, I'll let you decide.
| .whereIn('id', emailIds); | ||
| }; | ||
|
|
||
| const updateEmail = ({ |
There was a problem hiding this comment.
That might be an improvement, I'll let you decide.
2 participants
No description provided.