Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add UI for {imap,smtp}_certificate_checks options #1076

Merged
merged 5 commits into from
Nov 4, 2019
Merged

Add UI for {imap,smtp}_certificate_checks options #1076

merged 5 commits into from
Nov 4, 2019

Conversation

link2xt
Copy link
Collaborator

@link2xt link2xt commented Oct 25, 2019 •
edited
Loading

This PR is to solve #1036 issue

@link2xt link2xt mentioned this pull request Oct 26, 2019
Closed
@link2xt
Copy link
Collaborator Author

link2xt commented Oct 26, 2019

Waiting for #1082 to be merged first.

Also, configure function in deltachat-node/lib/deltachat.js needs to be extended to handle these options.

@link2xt
Copy link
Collaborator Author

link2xt commented Oct 27, 2019

Blocked by deltachat/deltachat-node#388 now

This was referenced Oct 29, 2019
Closed
Closed
cyBerta
cyBerta reviewed Oct 29, 2019
View reviewed changes
src/renderer/components/Login.js Outdated
@@ -206,6 +211,17 @@ export default class Login extends React.Component {
<option value='starttls'>STARTTLS</option>
<option value='plain'>{tx('off')}</option>
</DeltaSelect>
<DeltaSelect
id='imap_certificate_checks'
label={tx('login_imap_certificate_checks')}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the entry for login_imap_certificate_checks is missing in the sources, or?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup seems like the translations (at least experimental/untranslated) are missing.

@Jikstra
Copy link
Contributor

Jikstra commented Oct 31, 2019

@link2xt can you update the package.json to use the current deltachat-node git master? This way we can test master a couple of days without having a new -node release.

@link2xt
Copy link
Collaborator Author

link2xt commented Oct 31, 2019 •
edited
Loading

@Jikstra Done. Doing npm install and testing now, will try to rebase on top of master then.

Edit: had to rebase because otherwise the code does not compile since 05f7de6

Jikstra
Jikstra suggested changes Nov 1, 2019
View reviewed changes
Copy link
Contributor

@Jikstra Jikstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add translations to the untranslated/experimental_en.json file.

src/renderer/components/Login.js Outdated
@@ -206,6 +211,17 @@ export default class Login extends React.Component {
<option value='starttls'>STARTTLS</option>
<option value='plain'>{tx('off')}</option>
</DeltaSelect>
<DeltaSelect
id='imap_certificate_checks'
label={tx('login_imap_certificate_checks')}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup seems like the translations (at least experimental/untranslated) are missing.

@r10s
Copy link
Member

r10s commented Nov 1, 2019

i think there should be only one option for the certificate checks, there is no good reason to be strict on the one but not on the other; i think this was the consens on irc together with @hpk42 @adbenitez and others.

on android, @cyBerta just did a thing at deltachat/deltachat-android#1096

Screen Shot 2019-11-01 at 12 09 35

technically, the option sets both core options, the current state is driven by imap_certificate_checks

@link2xt
Copy link
Collaborator Author

link2xt commented Nov 1, 2019

Ok, I'll change the interface, add translations and mark it as ready for review once it works.

@link2xt link2xt marked this pull request as ready for review November 2, 2019 21:29
@link2xt
Copy link
Collaborator Author

link2xt commented Nov 2, 2019

I have merged two options into one similarly to Android and added strings to _untranslated_en.json. By the way translated strings are already on Transifex.

@link2xt link2xt requested a review from Jikstra November 3, 2019 22:37
@Simon-Laux
Copy link
Member

By the way translated strings are already on Transifex.

looks like we could do a transifex pull again.

Jikstra
Jikstra reviewed Nov 4, 2019
View reviewed changes
src/renderer/components/Login.js
// Change to certificate_checks updates certificate checks configuration
// for all protocols.
updatedCredentials = {
['imap_certificate_checks']: { $set: value },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The [] aren't needed in this case i think.

@Jikstra Jikstra merged commit 32700aa into deltachat:master Nov 4, 2019
@Jikstra Jikstra mentioned this pull request Nov 4, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cyBerta cyBerta cyBerta left review comments

@Jikstra Jikstra Jikstra requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@link2xt @Jikstra @r10s @Simon-Laux @cyBerta