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.

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

Revert commit b7ae96f as the bitshares/bitshares-ui#1338 is complete #1049

Merged
merged 7 commits into from Sep 3, 2018

Conversation

Projects
5 participants
@jmjatlanta
Copy link
Contributor

commented Jun 13, 2018

The issue bitshares/bitshares-ui#1338 is now complete. Now defaulting the parameter enable_subscribe_to_all to false

A bit of history: This parameter was documented incorrectly, which caused the ui to send true, which in turn caused the API server to spam the clients with unwanted notifications. Now that the bitshares-ui has been corrected, we can default this parameter to false and avoid sending these unwanted notifications.

PR for #752.

@oxarbitrage
Copy link
Member

left a comment

thanls @jmjatlanta

@abitmore

This comment has been minimized.

Copy link
Member

commented Jun 13, 2018

Please be careful since it will break old clients (due to quality control of bitshares-ui project, I guess quite some people are still using old versions).

@jmjatlanta

This comment has been minimized.

Copy link
Contributor Author

commented Jun 13, 2018

What steps can we take to minimize the impact? We'll have it in the release notes, but I'd imagine most ui users could care less about core changes.

We could delay this change to a later release. But I don't know if we want to do that or not.

Opinions? Other ideas?

@ryanRfox ryanRfox added this to To Do in Feature Release (201808) via automation Jun 13, 2018

@ryanRfox ryanRfox moved this from To Do to In Testing in Feature Release (201808) Jun 19, 2018

@ryanRfox ryanRfox moved this from In Testing to To Do in Feature Release (201808) Jun 19, 2018

@abitmore abitmore moved this from To Do to In Development in Feature Release (201808) Jun 25, 2018

@abitmore abitmore added this to New -Awaiting Core Team Evaluation in Project Backlog via automation Jul 21, 2018

@abitmore abitmore removed this from In Development in Feature Release (201808) Jul 21, 2018

@cogutvalera
Copy link
Member

left a comment

GUI is ready and required code in core is uncommented. Seems everything is fine here. Thanks !

@abitmore

This comment has been minimized.

Copy link
Member

commented Aug 17, 2018

Let's do it in next release. Need to resolve conflicts now.

@abitmore abitmore removed this from New -Awaiting Core Team Evaluation in Project Backlog Aug 17, 2018

@abitmore abitmore added this to In progress in Feature release (201810) via automation Aug 17, 2018

@jmjatlanta jmjatlanta dismissed stale reviews from cogutvalera and oxarbitrage via e02ad1f Aug 23, 2018

@@ -943,12 +942,11 @@ void application::set_program_options(boost::program_options::options_descriptio
("api-access", bpo::value<boost::filesystem::path>(), "JSON file specifying API permissions")
("plugins", bpo::value<string>(), "Space-separated list of plugins to activate")
("io-threads", bpo::value<uint16_t>()->implicit_value(0), "Number of IO threads, default to 0 for auto-configuration")
("enable-subscribe-to-all", bpo::value<bool>()->implicit_value(false),

This comment has been minimized.

Copy link
@abitmore

abitmore Aug 23, 2018

Member

I think the implicit value should be true which is more natural.

@abitmore
Copy link
Member

left a comment

See comment above.

@@ -38,7 +38,7 @@ namespace graphene { namespace app {
class application_options
{
public:
bool enable_subscribe_to_all = false;
bool enable_subscribe_to_all = true;

This comment has been minimized.

Copy link
@abitmore

abitmore Aug 27, 2018

Member

Don't change this one to true.

Sorry, my above comment was not clear enough. Expected behavior:

  • this option is disabled by default (if not specified in config.ini nor startup parameters);
  • if a user specified it in parameters or config.ini, if without =X (only --enable-subscribe-to-all but not --enable-subscribe-to-all=true), consider the value is true.

jmjatlanta added some commits Sep 1, 2018

@jmjatlanta

This comment has been minimized.

Copy link
Contributor Author

commented Sep 1, 2018

I believe I have done what you asked @abitmore but please check. Either I am doing something wrong, or there is a side-effect here.

  • Adding --enable-subscribe-to-all to the command line sets enable_subscribe_to_all to true which is expected.
  • Not specifying --enable-subscribe-to-all on the command line sets enable_subscribe_to_all to false which is expected.
  • Commenting out enable-subscribe-to-all in config.ini sets enable_subscribe_to_all to false which is expected.
  • Uncommenting enable-subscribe-to-all in config.ini and setting it to true sets enable_subscribe_to_all to true which is expected.
  • Uncommenting enable-subscribe-to-all in config.ini and setting it to false sets enable_subscribe_to_all to true which is not expected.

I believe that the default config.ini should be generated with the commented out line, but without the = as it is misleading. Is that correct, or am I doing something else incorrect?

@clockworkgr

This comment has been minimized.

Copy link
Member

commented Sep 1, 2018

If this was a dynamically typed language this is exactly the sort of behaviour I'd expect if the value of enable-subscribe-to-all in config.ini was stored as a string and then evaluated as a Boolean.

Seeing as this is C++, I have no idea

@abitmore

This comment has been minimized.

Copy link
Member

commented Sep 1, 2018

I guess @clockworkgr is correct. For example, the partial-operations option generated in config.ini is 1 but not true. Actually I don't remember if I've ever tried to set it to false.

@@ -410,7 +410,7 @@ void application_impl::startup()
}

if( _options->count("enable-subscribe-to-all") )
_app_options.enable_subscribe_to_all = _options->at("enable-subscribe-to-all").as<bool>();

This comment has been minimized.

Copy link
@abitmore

abitmore Sep 1, 2018

Member

Sorry, I didn't get it, what's wrong with _options->at("enable-subscribe-to-all").as<bool>();?

This comment has been minimized.

Copy link
@jmjatlanta

jmjatlanta Sep 1, 2018

Author Contributor

If you don't add the command line option, you get an index out of range error.

Update: Okay, I see what you mean. I'm fixing it.

@jmjatlanta

This comment has been minimized.

Copy link
Contributor Author

commented Sep 1, 2018

Okay, I have fixed the config.ini problem. It was an error in my logic, as @abitmore pointed out. Now the command line and config.ini file work as expected. A 1/0 or true/false can work in config.ini as expected.

@@ -409,8 +409,8 @@ void application_impl::startup()
_force_validate = true;
}

if( _options->count("enable-subscribe-to-all") )
_app_options.enable_subscribe_to_all = true;
if (_options->count("enable-subscribe-to-all"))

This comment has been minimized.

Copy link
@abitmore

abitmore Sep 3, 2018

Member

Why change the white-spaces? :P

This comment has been minimized.

Copy link
@jmjatlanta

jmjatlanta Sep 3, 2018

Author Contributor

:-P

@abitmore abitmore referenced this pull request Sep 3, 2018

Open

Review subscription API #777

4 of 9 tasks complete

@jmjatlanta jmjatlanta moved this from In progress to In Testing in Feature release (201810) Sep 3, 2018

@abitmore abitmore merged commit 2997d02 into develop Sep 3, 2018

3 checks passed

ci/dockercloud Your tests passed in Docker Cloud
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

Feature release (201810) automation moved this from In Testing to Done Sep 3, 2018

@jmjatlanta jmjatlanta deleted the jmj_issue_752 branch Nov 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.