-
Notifications
You must be signed in to change notification settings - Fork 428
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
Remove domain/catch hook errors #3775
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Codecov ReportBase: 82.81% // Head: 82.81% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## remove_domain/retries #3775 +/- ##
=========================================================
- Coverage 82.81% 82.81% -0.01%
=========================================================
Files 531 531
Lines 34095 34109 +14
=========================================================
+ Hits 28236 28246 +10
- Misses 5859 5863 +4
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
0e92859
to
7193bba
Compare
9ce54b3
to
5c606f7
Compare
This comment was marked as outdated.
This comment was marked as outdated.
3c1a56f
to
6768524
Compare
This comment was marked as outdated.
This comment was marked as outdated.
7193bba
to
e365b77
Compare
6768524
to
45d10d3
Compare
This comment was marked as outdated.
This comment was marked as outdated.
e365b77
to
471d300
Compare
45d10d3
to
0ee3dfb
Compare
This comment was marked as outdated.
This comment was marked as outdated.
0ee3dfb
to
f875416
Compare
small_tests_24 / small_tests / f875416 small_tests_25 / small_tests / f875416 ldap_mnesia_24 / ldap_mnesia / f875416 dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / f875416 ldap_mnesia_25 / ldap_mnesia / f875416 dynamic_domains_pgsql_mnesia_25 / pgsql_mnesia / f875416 dynamic_domains_mysql_redis_25 / mysql_redis / f875416 dynamic_domains_mssql_mnesia_25 / odbc_mssql_mnesia / f875416 internal_mnesia_25 / internal_mnesia / f875416 pgsql_mnesia_24 / pgsql_mnesia / f875416 elasticsearch_and_cassandra_25 / elasticsearch_and_cassandra_mnesia / f875416 pgsql_mnesia_25 / pgsql_mnesia / f875416 mysql_redis_25 / mysql_redis / f875416 riak_mnesia_24 / riak_mnesia / f875416 mssql_mnesia_25 / odbc_mssql_mnesia / f875416 |
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.
Looks good in general, I added minor comments.
@@ -251,3 +258,15 @@ unregister_subdomain(HostType, SubdomainPattern) -> | |||
[mongoose_subdomain_core:subdomain_info()]. | |||
get_all_subdomains_for_domain(Domain) -> | |||
mongoose_subdomain_core:get_all_subdomains_for_domain(Domain). | |||
|
|||
-spec remove_domain_wrapper(remove_domain_acc(), fun(() -> remove_domain_acc()), module()) -> |
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.
Maybe it's only me, but I think the name suggests that it removes a domain wrapper. No need to fix.
mongoose_rdbms:execute_successfully(HostType, mam_remove_domain_users, [Domain]) | ||
end), | ||
Acc. | ||
F = fun() -> |
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.
It is really unexpected that this function is (and most likely was already) untouched by tests. Could you investigate? I thought we had tests for domain removal for MAM.
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.
No idea, I just ran locally with coverage and it is green in the report generated here, I don't know why it wasn't for codecov 😕
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.
codecov says that for master is also red: https://app.codecov.io/gh/esl/MongooseIM/blob/master/src/mam/mod_mam_rdbms_arch.erl so well, I'm sure we have a test for it, in
mam_pm_removal(Config) -> |
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.
Ok, let's leave it for now
No description provided.