minimum visits and minduration not persisting when changed #5824
Conversation
| @@ -143,15 +143,14 @@ const doAction = (action) => { | |||
|
|
|||
| case settings.MINIMUM_VISIT_TIME: | |||
| if (action.value <= 0) break | |||
|
|
|||
| synopsis.options.minDuration = action.value * 1000 | |||
There was a problem hiding this comment.
in order for the ledger code to actually do the pruning, you'll need this line. just remove the "* 1000"
There was a problem hiding this comment.
@mrose17 this is seconds now right? (previously was milliseconds?) It does seem to be updated like you suggest (see next line)
| updatePublisherInfo() | ||
| break | ||
|
|
||
| case settings.MINIMUM_VISITS: | ||
| if (action.value <= 0) break | ||
|
|
||
| synopsis.options.minPublisherVisits = action.value |
There was a problem hiding this comment.
in order for the ledger code to actually do the pruning, you'll also need this line.
There was a problem hiding this comment.
@mrose17 ok so the problem right now in master is that it's first stored in ledgerData and then here it's added to ledgerData.synopsisOptions.
If it needs to be in synopsisOptions is the fix moving the initial value from the root of ledgerData?
|
Will check this out tonite and give feedback 😄 (and respond to your question about automated test) |
|
@bsclifton thanks for checking it out! @mrose17 how is this approach? What if we don't set the duration and the minimum visits on the root object? And instead only set it / look for it in synopsisOptions? If this doesn't work we can keep it in both places, it just seems unnecessary :) |
|
i agree it would be best to maintain the value in one place. in that case, i think that the place to keep the value is in |
|
@mrose17 ok so take minDuration and minPublisher visits out of synopsis.options and move them into synopsis? |
|
The ledger code is distinctly different than the rest of the codebase, which can cause confusion (or at least it does for me). I think with a little work, we can make this more Redux friendly (and also unit-testable). Give me a few mins to cook up a change... |
|
For this PR, it sounds like we need to update Would you guys be able to work this out on Slack? 😄 Or if it's easier, we can do a Google hangout tomorrow (let me know, I'll set it up) |
|
@mrose17 LGTM! @bsclifton assuming you ++ I think this one is ready to merge! |
|
agreed. @bsclifton - you get the "final say" ! |
| appActions.changeSetting(settings.MINIMUM_VISIT_TIME, value) | ||
| } | ||
| if (value > 0) synopsis.options.minDuration = value * 1000 | ||
| // for earlier versions of the code... | ||
| if ((value > 0) && (value < 1000)) synopsis.options.minDuration = value * 1000 |
|
@jkup @mrose17 LGTM! Thanks for working it out 😄 I'll check out the tests here and merge It could use a unit test to help prevent any regressions or unintended altering of the behavior 😊 @jkup if you would be willing to check that out, you could address with a follow up 😄 (logic could be broken out into ledgerUtil for easy testing). Shouldn't be too bad, just checking the value of |
|
Tests finished running; the ones which failed on travis-ci passed when I ran them locally. Merging! 😄 |
|
@bsclifton thanks for the merge! created this #5887 |
git rebase -ito squash commits (if needed).Fix #4822
Auditors @mrose17 @bsclifton
Two things here. One, we were multiplying by 1000 before storing the value but that causes a bug when the (now * 1000) value gets back to the UI. So instead, let's just keep the values in milliseconds.
The second thing is we initially store the minDuration and minimumVisits in ledgerData at the root but then we were saving it to ledgerData.synopsisOptions when it changed so the UI was grabbing the old value!
@bsclifton can you think of a good automated test for this? I know we chatted about it yesterday but I was struggling to implement something that makes sense!
Test Plan