-
Notifications
You must be signed in to change notification settings - Fork 165
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.
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
Refact #1099 to make tests work #1101
Conversation
@Shadow243, it is really a bug. You have 4 or more IMAP accounts, but you have not configured any. |
} | ||
$default_server_id = $id; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why default_server_Id is every ID in the imap list? Shouldn't it be only the one marked as default or maybe the first one?
@@ -146,7 +146,10 @@ public function filter_servers() { | |||
foreach ($this->config as $key => $vals) { | |||
if (in_array($key, $excluded, true)) { | |||
foreach ($vals as $index => $server) { | |||
if (!array_key_exists('server', $server)) { | |||
if (!empty($server['default'])) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This returns the error we wanted to fix. The goal of removing this code was to allow you to have the same unique identifier for the default server each time you reconnect. This resulted in losing the configured folders. With this we will have the same problem.
@Shadow243 I might be a different issue. Cypht standalone is working fine. You need to fix the issue in Tiki |
@josaphatim I had the same issue with Cypht standalone. |
I am a bit lost here, can @josaphatim or @Shadow243 summarize what the original bug was, how it was fixed and why this current PR is needed? Some tests were failing after the original fix? |
} else { | ||
// Perhaps something as changed | ||
Hm_IMAP_List::edit($default_server_id, $imap_details); | ||
if (!$default_server_id) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change https://github.com/cypht-org/cypht/pull/1099/files#diff-b7fdf5637005ea55ecbcc642d9660fa0fe278967cd7ce7ac7dbd9bfa7c4e0ed8R149 affected the tests, upon rechecking I thought that a refactoring was necessary for the tests to pass without affecting the correction in the PR: #1099