Conversation
|
@Kharchevskyi please have a look at the rendering parts. Thanks! |
|
In general looks good 👍 |
|
I should also clarify: as long as the visual doesn't look broken, some ugliness is OK at this stage. Functionality is a lot more important at this stage then looks. There will be a time that we refurbish the app for looks sometime in 2022 but this year we won't have time for that, so a few rough edges in UI are ok. This also applies to PR reviews - the first focus is functionality and correctness (and usability), and visual appeal is secondary. |
|
@Kharchevskyi how do you propose this type of functionality should be tested? |
|
As I understand there is only UI added in this PR. |
Definitely - once functionality is in place, what should be the way to test this? With you answer, we should probably update readme to document a clear overview of our approach to testing. |
|
@tomholub Testing is quite specific task by task. For this task I think expected result is that Attachment block is present in messages where they are. They coloured in correct colour. Toast is shown when tapping on attachment. |
Yes, but more specifically - are we pulling messages from live Gmail API to test this? Or do we mock it? Or a unit test? This is what we need to make some standardization and guidance for, else contributors will not know what type of test to write and how. |
|
Yes, we use Gmail API to test functionality. Unit test - good question. We do not have a lot of unit tests now. But I do think we need to work on it and add some scope in each PR for writing tests. Or even cover some previous implementation with tests |
|
We don't have any mock API available for iOS testing yet. On Android, we test though IMAP where we have a mock server. That produces a pretty decent testing ergonomy. While it doesn't test the Gmail API vs IMAP/SMTP layer, it's pretty small difference. That means for this test, we would test by opening an actual Gmail message, on live Gmail servers like other tests, and testing through the UI - because that's about the only thing we are set up for, for now. correct? |
|
Is it possible to somehow share this IMAP server? Yes, for now all tasks like that can be tested only with real Gmail account. Usually I use accounts from `test-ci-secrets.json" |
|
Let's skip tests on this PR, and we'll do manual testing for this one. Then we'll set up tests afterwards with mock IMAP server. |
|
@qsdemir how is this coming along, could we get this in mergeable state soon? |
|
@tomholub @Kharchevskyi Hi, can you please take a look into this again? |
|
@qsdemir you can mark the PR ready for review if you think it's good for review / potential merging. @seisvelas please build and run this to check the functionality @Kharchevskyi please have a look at the code |
| private let messageProvider: MessageProvider | ||
| private let messageOperationsProvider: MessageOperationsProvider | ||
| private var message: NSAttributedString | ||
| private var attachments: [Attachment] |
There was a problem hiding this comment.
you can initiate it here instead of inside init private var attachments: [Attachment] = []
| isEmail: true | ||
| ) | ||
| let decryptErrBlocks = decrypted.blocks.filter { $0.decryptErr != nil } | ||
| let decryptAttBlocks = decrypted.blocks.filter { $0.type == .plainAtt || $0.type == .encryptedAtt || $0.type == .decryptedAtt } |
There was a problem hiding this comment.
I would suggest to have some computed property for MsgBlock and cover it with tests(not in scope of this PR)
extension MsgBlock {
var isAttachmentBlock: Bool {
type == .plainAtt || type == .encryptedAtt || type == .decryptedAtt
}
}
There was a problem hiding this comment.
It would be called isAttachmentBlock. But this is rather minor as you say
| let decryptErrBlocks = decrypted.blocks.filter { $0.decryptErr != nil } | ||
| let decryptAttBlocks = decrypted.blocks.filter { $0.type == .plainAtt || $0.type == .encryptedAtt || $0.type == .decryptedAtt } | ||
|
|
||
| let attachments = decryptAttBlocks.map { Attachment(name: $0.attMeta?.name ?? "Attachment", size: $0.attMeta?.length ?? 0) } |
There was a problem hiding this comment.
same for Attachment. You can have
extension Attachment {
init(block: MsgBlock) {
self.name =
self.size =
}
}
and then it can be just
let attachments = decrypted.blocks
.filter(\.isAttachmentBlock)
.map(Attachment.init)
this solution is more flexible and can be tested easily.
| attachmentNodes = attachments.map { AttachmentNode(input: AttachmentNode.Input(name: $0.name, size: $0.size), | ||
| onTap: { | ||
| self.onTap?() | ||
| }) | ||
| } |
There was a problem hiding this comment.
please change indentation here? hard for understanding
attachmentNodes = attachments.map {
input: AttachmentNode(
input:
onTap:
)
)
}
There was a problem hiding this comment.
Can xcode be set to indent automatically on save?
There was a problem hiding this comment.
We already have a script for that. Will check if it's possible to indent code during build phase.
Will work on it ASAP to prevent all this "code style" comments
|
@qsdemir good job 👍 |
@tomholub Thanks. I was not aware of this button Redy for review. |
|
I just realized @seisvelas cannot run this due to #285 . I'll test this myself for now until that is resolved. |
tomholub
left a comment
tomholub
left a comment
There was a problem hiding this comment.
I've tested the functionality, looks good. We can address the style and simplification review points made by Anton in another PR later.
4 participants
This is a draft PR to check whether or not we're on a good path. Please note that most issues come with using Texture, since we're unfamiliar with it, so here you can find two attachments hardcoded and shown in every email just to check if nodes and this approach are correct.