Remove expired polls from overview page #1250

personthingman2 · 2018-08-08T01:25:49Z

This is my first contribution to an open source project so please tell me if I'm doing anything wrong.
This is a fix for #1168. I moved the logic into GetGlobalStatus because MemorizeMessage is only run when a message is received, meaning users would have to restart their client for an expired poll to disappear. As a result msPoll now contains the entire message XML so it can be processed in GetGlobalStatus.
The "Poll: " and "Foundation Poll: " concats were removed because it looked silly to me.

tomasbrod

Please put your logic in a function and wrap it in a try-catch so it does not crash wallet on invalid data.
Otherwise good. Thank you for contributing.

tomasbrod · 2018-08-08T07:31:56Z

src/main.cpp

+        // Alerts are displayed as polls but do not have an expiration
+        if(sPollExpiration.empty())
+        {
+            sPollExpiration = std::to_string(pindexBest->nTime);


Avoid converting things to string if possible.

tomasbrod · 2018-08-08T07:32:38Z

src/main.cpp

+        {
+            sPollExpiration = std::to_string(pindexBest->nTime);
+        }
+        if (stoll(sPollExpiration) >= pindexBest->nTime)


Unsafe strtoll. Please add error and exception handling or use another function (ToString maybe?).

tomasbrod · 2018-08-08T07:33:49Z

src/main.cpp

+        }
+        else
+        {
+            GlobalStatusStruct.poll = "No current polls";


I do not like this.

Could you elaborate? I don't see what's wrong with this.

…ed exception handling for stoll()

personthingman2 · 2018-08-10T20:03:30Z

I've moved the logic into the new functionGetPoll(), added uint_64_t uPollExpiration which removes the need to convert to string, and put a try/catch around the remaining stoll(). I don't believe an additional try/catch is required around the function call in GetGlobalStatus() because that entire function is already in a try/catch.

iFoggz · 2018-08-10T21:23:40Z

I agree with @tomasbrod asking of the try catch there as well. better to pass back no current poll for malformed time then having msMiningErrors saying Error obtaining status and in turn not providing the other valid data/or incomplete data to the wallet -- this saves confusion as well as to where the problem is/was

tomasbrod · 2018-08-11T13:59:05Z

src/main.h

@@ -260,6 +260,8 @@ bool WriteKey(std::string sKey, std::string sValue);
 double GetBlockDifficulty(unsigned int nBits);
 std::string ExtractXML(std::string XMLdata, std::string key, std::string key_end);

+std::string GetPoll();


Please use more descriptive name, with overview or statusbar words. There are getters for rpcs and voting tab, we do not want name clashes.

tomasbrod

I accidentally set status to approve.
About that no polls message: if it is a separate line, sure, show that message so it does not appear empty. But if it is under status field, no polls message would just takes up space.

…around function call

personthingman2 · 2018-08-11T21:26:20Z

I've changed GetPoll() to GetCurrentOverviewTabPoll() and added a try/catch around the function call with a log error.

The "No current polls" message is only shown in the "Current Poll: " field of the overview tab so it does not clutter any other field.

Here is a screenshot of what it looks like:

iFoggz · 2018-08-11T23:16:05Z

label poll is not used for anything but poll

iFoggz

tACK 569bc8d; apon another approval I think due to this being a cosmetic fix more then anything it'll be a small include for the upcoming release milestone if @denravonska and @tomasbrod concur.

iFoggz · 2018-08-11T23:27:03Z

src/main.cpp

+        }
+        catch (std::exception &e)
+        {
+            GlobalStatusStruct.poll = "Error obtaining last poll";


Translations should be considered here.
GlobalStatusStruct.poll = _("Error obtaining last poll");

Why the user needs to see this error?

I agree if anything a log entry about the exception should be put. Remove translation and log the e.what() under debug

iFoggz · 2018-08-11T23:27:32Z

src/main.cpp

+    }
+    else
+    {
+        poll = "No current polls";


Translations:
poll = _("No current polls");

What about "Current Poll: none"?

Short but sweet.

iFoggz

Actually thinking about this. We should consider use the _() and do translations where we can to increase the readability across languages where we can.

personthingman2 · 2018-08-14T17:13:14Z

I will wait on adding translations until #1262 is merged.

Do we want it to say "No current polls" or "None"? I think the former sounds better in the context of the overview page.

iFoggz · 2018-08-14T23:38:27Z

i would say don't worry about the translations part and it can be done at a later point when we do another translation update before release :) having the _() wont break your code. no worries there -- so just add the requested _() in a commit and make sure all reviews are satisfied and we can move to approval and merge. i'll update the translation strings at next release

you wouldn't be manually adding to the bitcoinstrings.cpp file. there's a script for that and would be in a separate pr for updated information

…the GUI

personthingman2 · 2018-08-28T01:52:06Z

Is this PR appropriate in its current state or would you guys like any additional changes?

cyrossignol · 2019-03-10T19:32:23Z

Any reason to worry about the full contract message appearing in the output of getmininginfo now for command-line users?

"MiningInfo 2": "<MT>poll</MT><MK>What_do_you_call_a_positively_charged_seal?\n</MK><MV><TITLE>What_do_you_call_a_positively_charged_seal?\n</TITLE><DAYS>14</DAYS><QUESTION>What_do_you_call_a_positively_charged_seal?\n</QUESTION><ANSWERS>A_Sealion;God_dammit_Foxifi!</ANSW ERS><SHARETYPE>3</SHARETYPE><URL>https://i.imgur.com/1D4iQZl.jpg</URL><EXPIRATION>1544712997</EXPIRATION></MV><MA>A</MA><MPK></MPK><MS>MEQCIHwhNJRKus9MwSYWo6epSyfaNytv2/IFFp5fCVUDNZwUAiBUS2U68h4y+ptkQ7tnUr/YGO8bL1Gz4EbIrE1207g2zA==</MS>",

philipswift · 2019-03-11T13:40:27Z

With jokes like that...yes! ;)

@jamescowens

Added: - Add `rainbymagnitude` RPC command #1235 (@Foggyx420). - Add stake splitting and side staking #1265 (@jamescowens). - Detect and block Windows shutdown so wallet can exit cleanly #1309 (@jamescowens). - Add message support to sendfrom and sendtoaddress #1400 (@denravonska). Changed: - Configuration options are now case insensitive #294 (@Foggyx420). - Update command in beaconstatus help message #1312 (@chrstphrchvz). - Improve synchronization speeds: - Refactor superblock pack/unpack #1194 (@denravonska). - Optimize neuralsecurity calculations #1255 (@denravonska). - Reduce hash calculations when checking blocks #1206 (@denravonska). - Make display of private key in beaconstatus OPT-IN only #1275 (@Foggyx420). - Store Beacon keys in Wallet #1088 (@tomasbrod). - Use default colors for pie chart #1333 (@chrstphrchvz). - Show hand cursor when hovering clickable labels #1332 (@chrstphrchvz). - Update README.md #1337 (@Peppernrino). - Fix integer overflow with displayed nonce #1297 (@personthingman2). - Improve application cache performance #1317 (@denravonska). - Improve reorg speeds #1263 (@denravonska). - Update Polish translation #1375 (@michalkania). Fixed: - Remove expired polls from overview page #1250 (@personthingman2). - Fix plural text on block age #1304 (@scribblemaniac). - Fix researcher staking issue if your chain head was staked by you, #1299 (@denravonska). - Fix incorrect address to grcpool node #1314 (@wilkart). - Do not replace underscores by spaces in Qt Poll URLs #1327 (@tomasbrod). - Fix scraper SSL issues #1330 (@Foggyx420). Removed: - Remove or merged several RPC commands #1228 (@Foggyx420): - `newburnaddress`, removed. - `burn2`: Removed. - `cpid`: Merged into `projects`. - `mymagnitude`: Merged into `magnitude`. - `rsa`: Removed, use `magnitude`. - `rsaweight`: Removed, use `magnitude`. - `proveownership`: Removed. - `encrypt`: Removed. - Remove obsolete POW fields from RPC responses #1358 (@jamescowens). - Remove obsolete netsoft fields for slight RAM requirement reduction #1336 (@denravonska). - Remove unused attachment functionality #1345 (@denravonska).

Remove expired polls from overview page

889182d

tomasbrod suggested changes Aug 8, 2018

View reviewed changes

Moved into new function GetPoll(), removed string conversion, and add…

6826fe9

…ed exception handling for stoll()

tomasbrod approved these changes Aug 11, 2018

View reviewed changes

tomasbrod suggested changes Aug 11, 2018

View reviewed changes

Changed GetPoll() to GetCurrentOverviewTabPoll() and added try/catch …

569bc8d

…around function call

iFoggz approved these changes Aug 11, 2018

View reviewed changes

iFoggz reviewed Aug 11, 2018

View reviewed changes

iFoggz suggested changes Aug 11, 2018

View reviewed changes

Added _() for translations, Log errors instead of displaying them in …

6d19b7a

…the GUI

tomasbrod approved these changes Aug 28, 2018

View reviewed changes

iFoggz approved these changes Aug 28, 2018

View reviewed changes

denravonska merged commit d638c81 into gridcoin-community:development Sep 3, 2018

denravonska added this to the Camilla milestone Oct 8, 2018

denravonska added the bug label Oct 8, 2018

This was referenced Oct 8, 2018

Expired last poll is displayed under "Current poll" on Overview tab #1168

Closed

Expired poll is shown at overview as current poll #550

Closed

a123b mentioned this pull request Apr 16, 2019

Restore old poll output for getmininginfo RPC #1437

Merged

cyrossignol mentioned this pull request May 7, 2019

Fix missing poll in GUI when most recent poll expired #1455

Merged

Remove expired polls from overview page #1250

Remove expired polls from overview page #1250

Uh oh!

Conversation

personthingman2 commented Aug 8, 2018

Uh oh!

tomasbrod left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

personthingman2 commented Aug 10, 2018

Uh oh!

iFoggz commented Aug 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomasbrod left a comment

Choose a reason for hiding this comment

Uh oh!

personthingman2 commented Aug 11, 2018

Uh oh!

iFoggz commented Aug 11, 2018

Uh oh!

iFoggz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iFoggz left a comment

Choose a reason for hiding this comment

Uh oh!

personthingman2 commented Aug 14, 2018

Uh oh!

iFoggz commented Aug 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

personthingman2 commented Aug 28, 2018

Uh oh!

cyrossignol commented Mar 10, 2019

Uh oh!

philipswift commented Mar 11, 2019

Uh oh!

Uh oh!

iFoggz commented Aug 10, 2018 •

edited

Loading

iFoggz commented Aug 14, 2018 •

edited

Loading