[00:04:23] Welp, gotta run. Let me know how it comes out. [00:51:11] (PS7) Ejegg: Actually split out config classes [wikimedia/fundraising/SmashPig] - https://gerrit.wikimedia.org/r/355577 [02:26:27] (PS8) Ejegg: Actually split out config classes [wikimedia/fundraising/SmashPig] - https://gerrit.wikimedia.org/r/355577 (https://phabricator.wikimedia.org/T143831) [02:31:07] (PS1) Ejegg: Always (re-)initialize logger when setting provider configuration [wikimedia/fundraising/SmashPig] - https://gerrit.wikimedia.org/r/355739 (https://phabricator.wikimedia.org/T143831) [02:33:34] Fundraising-Backlog, Wikimedia-Fundraising, MW-1.30-release-notes (WMF-deploy-2017-05-30_(1.30.0-wmf.3)), Patch-For-Review: Missing interface messages on donatewiki - https://phabricator.wikimedia.org/T166302#3293763 (Ejegg) Open>Resolved a:Ejegg OK, this is looking good in multiple l... [02:34:11] Fundraising Sprint Kickstopper, Fundraising-Backlog, Wikimedia-Fundraising, MW-1.30-release-notes (WMF-deploy-2017-05-30_(1.30.0-wmf.3)), Patch-For-Review: Missing interface messages on donatewiki - https://phabricator.wikimedia.org/T166302#3293766 (Ejegg) [09:33:18] Fundraising-Backlog, fundraising-tech-ops: Civi & Server access for Moska Noor - https://phabricator.wikimedia.org/T164040#3294128 (CCogdill_WMF) Hi @cwdent, Moska and I sat down to do this today and I think we're missing a step in the process. After entering the ssh-keygen command, we were prompted with... [09:40:18] Fundraising-Backlog, fundraising-tech-ops: Civi & Server access for Moska Noor - https://phabricator.wikimedia.org/T164040#3294143 (CCogdill_WMF) Spoke to soon, kinda. We went ahead and tried a .txt file, but got this passphrase error (line 4): Enter file in which to save the key (/Users/mnoor/.ssh/id_... [13:13:19] (PS1) Umherirrender: Add explict var visibility [extensions/CentralNotice] - https://gerrit.wikimedia.org/r/355767 [14:24:42] (PS6) Mepps: Log error if Minfraud service not reached and add configured risk score [extensions/DonationInterface] - https://gerrit.wikimedia.org/r/355480 (https://phabricator.wikimedia.org/T150072) [14:34:04] (CR) jerkins-bot: [V: -1] Log error if Minfraud service not reached and add configured risk score [extensions/DonationInterface] - https://gerrit.wikimedia.org/r/355480 (https://phabricator.wikimedia.org/T150072) (owner: Mepps) [15:21:41] (PS7) Krinkle: Rename some impressionDiet variables [extensions/CentralNotice] - https://gerrit.wikimedia.org/r/319003 (https://phabricator.wikimedia.org/T121178) (owner: Ejegg) [15:22:11] (PS8) Krinkle: Rename some impressionDiet variables [extensions/CentralNotice] - https://gerrit.wikimedia.org/r/319003 (https://phabricator.wikimedia.org/T121178) (owner: Ejegg) [15:50:11] PROBLEM - check_puppetrun on frlog1001 is CRITICAL: CRITICAL: Puppet has 19 failures. Last run 6 minutes ago with 19 failures. Failed resources (up to 3 shown) [15:55:11] RECOVERY - check_puppetrun on frlog1001 is OK: OK: Puppet is currently enabled, last run 49 seconds ago with 0 failures [16:53:21] mepps: so our fraud filter test had minfraud enabled all along and was just relying on the failure score of 100? [16:53:48] apparently [16:53:53] blecch [16:54:07] yeah.. [16:54:27] well, if we're going to test a minfraud failure let's at least point it at a nonsense url on localhost [16:54:31] or a malformed URL [16:54:53] yeah and be more explicit in what's being tested [16:54:58] right [16:55:15] i'd say we split out the failure mode from the other tests [16:55:28] so, you can selectively turn off minfraud in the test setup [16:56:33] Fundraising-Backlog, fundraising-tech-ops: Civi & Server access for Moska Noor - https://phabricator.wikimedia.org/T164040#3295015 (cwdent) @CCogdill_WMF @MNoorWMF ``` Enter file in which to save the key (/Users/mnoor/.ssh/id_rsa): ``` The part in parentheses is the default value, which is fine, so jus... [16:58:10] so create a class just for minfraud tests? [16:58:30] mepps errr [16:58:58] or you could reset the global right in the minfraud test [16:59:11] and I think the teardown will set it back [17:00:19] fr-tech: San Francisco isn't what it used to be, and it never was. [17:00:19] -- Herb Caen [17:00:19] -- discuss. [17:00:30] heh [17:00:37] the future isn't what it used to be [17:00:45] haha [17:00:50] Fundraising-Backlog, Analytics, MediaWiki-extensions-CentralNotice, Performance-Team, Spike: Spike: Investigate alternatives to Special:HideBanners cookie storm for cross-domain banner close-button - https://phabricator.wikimedia.org/T117433#3295037 (Krinkle) p:Triage>Normal [17:01:43] ejegg i'm not sure i understand what you're suggesting--what i'm hearing is that you'd like there to be a flag to skip minfraud tests? [17:02:28] mepps sorry, I'm undecided myself, just throwing out possibilities [17:03:18] easiest would be to explicitly set the url to something malformed and set the failure score and weight [17:03:21] ejegg oh gotcha :) yeah i guess i'm wondering if it was relying on the failure mode just to have a dummy score for tests? [17:03:24] all in the setup of the existing test [17:03:55] that's obviously not the behavior we'd want, it might sense just to set a score then or be clear that we're testing for failure [17:04:14] sorry i mean what's existing is not what we want, not what you suggested :) [17:05:38] Fundraising-Backlog, fundraising-tech-ops: Civi & Server access for Moska Noor - https://phabricator.wikimedia.org/T164040#3295061 (CCogdill_WMF) Oh, I thought she had to hit the yubikey as her password. That must have been the issue. @MNoorWMF, can you give it another shot? [17:13:38] oh i found where this is happening! [17:13:57] it's in query_minfraud: if ( $this->gateway_adapter->getGlobal( 'Test' ) ) { [17:13:57] $this->minfraudResponse = 0; [17:13:57] } else { [17:14:08] ewwww [17:14:18] yup... [17:14:59] feel free to kill that [17:15:59] yeah i just deleted it, now trying to figure out how to stub out for tests [17:37:27] (PS5) AndyRussG: Purge banner content from front-end cache on banner save [extensions/CentralNotice] - https://gerrit.wikimedia.org/r/336237 (https://phabricator.wikimedia.org/T154954) [17:39:22] mepps ooh, curl can use file:// urls! [17:39:39] that could be handy for mocking all kinds of things [17:40:51] would it work for post tho? [17:41:50] eh, no headers [17:43:18] sorry, false alarm [17:51:38] (PS7) Mepps: Log error if Minfraud service not reached, add configured risk score, update test [extensions/DonationInterface] - https://gerrit.wikimedia.org/r/355480 (https://phabricator.wikimedia.org/T150072) [17:53:56] (CR) jerkins-bot: [V: -1] Log error if Minfraud service not reached, add configured risk score, update test [extensions/DonationInterface] - https://gerrit.wikimedia.org/r/355480 (https://phabricator.wikimedia.org/T150072) (owner: Mepps) [18:17:51] (CR) AndyRussG: Purge banner content from front-end cache on banner save (1 comment) [extensions/CentralNotice] - https://gerrit.wikimedia.org/r/336237 (https://phabricator.wikimedia.org/T154954) (owner: AndyRussG) [18:44:16] ejegg is there a different procedure to rerun build if a change hasn't been +2ed? [18:46:15] the comment 'recheck' might help [18:54:52] (CR) Mepps: "Recheck." [extensions/DonationInterface] - https://gerrit.wikimedia.org/r/355480 (https://phabricator.wikimedia.org/T150072) (owner: Mepps) [18:58:34] thanks that worked! [19:00:29] (PS8) Mepps: Log error if Minfraud service not reached, add configured risk score, update test [extensions/DonationInterface] - https://gerrit.wikimedia.org/r/355480 (https://phabricator.wikimedia.org/T150072) [19:01:18] awesome! [19:13:14] (PS9) Ejegg: Log error if Minfraud service not reached, add configured risk score, update test [extensions/DonationInterface] - https://gerrit.wikimedia.org/r/355480 (https://phabricator.wikimedia.org/T150072) (owner: Mepps) [19:14:17] (CR) Ejegg: [C: 2] "Thanks, this looks great!" [extensions/DonationInterface] - https://gerrit.wikimedia.org/r/355480 (https://phabricator.wikimedia.org/T150072) (owner: Mepps) [19:16:38] (Merged) jenkins-bot: Log error if Minfraud service not reached, add configured risk score, update test [extensions/DonationInterface] - https://gerrit.wikimedia.org/r/355480 (https://phabricator.wikimedia.org/T150072) (owner: Mepps) [19:16:39] hmm, still something missing in the configuration refactor [19:19:22] provider configurations are still created with new() [19:20:12] which will make them hard to manipulate when we're testing code that sets them [19:25:23] mepps any suggestions? [19:25:48] i guess we could do something like put a 'ProviderConfigurationFactory' in the global config [19:26:17] then under test, swap that out with a factory that hands out mock versions [19:26:36] but that feels unweildy [19:27:23] I also feel like the line between which keys are in global and which are in provider is a bit arbitrary [19:27:43] mostly the fact that CurlWrapper is in provider while cache is in global [19:27:59] hmm how did you think about it when you were breaking them out? [19:28:42] if we've ever overridden something for a single processor, I put that in the provider config [19:29:32] along with the things that only make sense for a provider, like the IPN class settings [19:29:39] (instant payment notifier) [19:30:09] i.e. the http endpoint we host to hear about payment status updates [19:33:19] the SmashPig tests are all pretty unit-y, but there are a bunch of integration style tests in DonationInterface that cover the whole php request, from special page construction to html rendering [19:33:38] and it's those integration tests I'm thinking about now [19:35:14] we should be able to set the global config early enough that tests can replace it for the whole process [19:36:29] so I guess obtaining the processor config will have to involve the global config at some level [19:39:38] cwd: feel like giving any input on the config issue? [19:39:57] only if you're not busy with ops stuff [19:40:18] hey, sure, lemme read backscroll [19:40:57] cwd so the context is this patch: https://gerrit.wikimedia.org/r/355577 [19:41:16] where we try to split config into global and processor [19:41:29] sort of like the global vs job config in process-control [19:43:38] the motivation was partly to be able to set the global config before knowing which processor we're dealing with [19:43:58] makes sense [19:44:02] for server side stuff [19:44:50] but i imagine there are a lot of things in between global and processor config? [19:45:12] one wrinkle is that there are some settings like logging which we need to use early on, but which we want to be able to override per processor [19:45:55] oops, standup [20:09:00] mepps sorry, were you asking something? [20:09:23] ejegg, just wanted to say if there was any more specific feedback you wanted on configuration update [20:09:31] to see if, not say if :) [20:10:32] well, I started in on it because I wanted to finish up this train of patches in DonationInterface: https://gerrit.wikimedia.org/r/355453 [20:10:53] to get rid of the queue code there and just use SmashPig's QueueWrapper [20:11:43] but once I got to that end patch, I realized the current state of config in SmashPig was going to make those integration tests impossible [20:12:58] So I guess I'm specifically looking for a way to get the SmashPig config into a good state for mocking all the things in a DonationInterface test [20:15:03] and the stickiest point there is how to sub in a provider configuration that we can control in tests [20:15:33] ok, i'm going to grab some food. back soon! [20:15:37] enjoy! [20:19:09] gotcha it's helpful to see that DI patch [21:24:54] as far as logging i like the approach of manually tagged log lines [21:25:26] if i remember correctly smashpig suffers from the tags being too automated? [21:29:51] cwd there's a log 'context' [21:29:55] that you can enter and leave [21:30:15] but you have to keep track of how deep you are [21:30:43] anyway, you can explicitly do that [21:31:27] but it changes per processor? [21:31:35] why is it a problem to init logging at the global level? [21:32:04] so, we want to be able to turn debug on for certain processors [21:32:25] which should definitely be possible without making every bit of logging processor-specific [21:32:54] but the way it's working now, we have all the logging config in one place [21:33:14] ah i see what you're saying [21:33:28] don't want to put processor config there [21:34:22] i'm trying not to get too hung up on logging since we should really just use monolog anyway [21:35:27] i guess the logger could be created from a global set of streams etc, and just check one setting in the current ambient processor config to determine which level actually gets through [21:35:42] so the processor config just includes logLevel [21:37:03] ehh [21:37:33] AndyRussG: do you run the qunit tests via mediwiki's phpunit wrapper? [21:38:30] or just with the qunit cli? [21:38:45] and if cli, what options do you use for --code ? [21:43:25] from an inheritance perspective log level is like an "abstract" property [21:43:42] from a top down mentality it's probably another config file? [21:43:45] ejegg: just from the browser [21:43:51] AndyRussG: oh right! [21:43:57] I'd just come across that page :) [21:44:39] there is all that processor specific config in donation interface [21:44:42] ejegg: something like localhost/wiki/index.php/Special:JavaScriptTest?module=ext.centralNotice.bannerSequence [21:44:55] Also debug=true works there [21:44:59] cwd I think in a monolog world it would be different levels for different channels [21:45:06] thanks AndyRussG [21:45:24] likewise! [21:45:36] cwd yeah, supporting all of the processor-specific config is definitely a goal of that patch [21:45:53] the ProcessConfiguration will eventually read in the var_map.yaml etc [21:46:52] are you planning to roll all that stuff into smashpig or have smashpig be able to read arbitrary kinds of config files? [21:47:05] roll it all in [21:47:24] since that stuff is pretty integral to the processor calls [21:47:50] in DI, one of the few things we /don't/ override per processor is logging [21:48:14] we just have a custom formatter that gets a prefix string from the current gateway [21:49:41] but DI will still have a role as the forms layer for the foreseeable future? [21:51:40] cwd yeah, I think so [21:52:29] seems like there will inevitably be some processor specific config in there too [21:52:45] just for forms? [21:53:15] maybe i'm wrong but it seems to always creep in [21:53:45] so far we've been able to keep that to the JS and the special page classes [21:53:57] Unless you include things like redirect URLs [21:56:23] seems like there is a lot of stuff in the yaml files that relates to the back end in one way or another [21:57:34] right, I'm hoping to move all of those to SmashPig [21:58:11] so DI config will just be mediawiki globals, and will only have to do with rendering forms [21:59:05] so an adapter layer that translates from smashpig to mediawiki? [21:59:21] yep, pretty much! [21:59:42] yeah that seems like a good plan to me [22:27:36] XenoRyet: where did you end up with that paypal ec session logging? [22:27:52] anything in particular missing that would help solve the puzzle? [22:28:27] Oh, that had kind of slipped my mind. I don't remember if I got anywhere with that. [22:29:42] we should probaby do another ec test soon [22:30:05] get that solidly in the rear view mirror [22:30:21] Yea, probably so, but I don't think anything jumped right out at me in terms of additional logging. Anything you can think of? [22:31:01] I didn't spend much time investigating the last test, but I can take a look after I review these qunit tests [22:31:49] Cool. I think I'm gonna try to stay in my current head space a while longer. [22:37:33] AndyRussG: so the second argument to QUnit.test is the number of assertions that should be made, huh? All the docs I can see have just two args [22:53:35] ejegg: yeah I couldn't find it in the docs, and it isn't required... But there are plenty of tests that do that [22:53:53] viz. other CN QUnit modules... [22:54:08] nice shorthand for assert.expect, i guess! [22:54:22] Yeah! Wonder why it isn't in the docs, hope it isn't deprecated [22:56:19] ejegg: hmmm... https://qunitjs.com/upgrade-guide-2.x/#replace-expected-argument-in-qunit-test [22:56:33] Maybe we should fix that then heh [22:56:48] oho! [22:57:00] hide the chickens! [22:57:22] what? no, no, only freshly laid eggs here, no chickens whatsoever [22:57:28] can definitely be a different PS [22:57:56] or rather different commit [22:58:03] I guess we should purge it alloverplaces [23:09:10] (CR) Ejegg: [C: 2] QUnit tests for ext.centralNotice.bannerSequence [extensions/CentralNotice] - https://gerrit.wikimedia.org/r/354379 (https://phabricator.wikimedia.org/T144456) (owner: AndyRussG) [23:09:43] oops, meant to comment & rebase [23:10:06] tiredness + eye-catching wrong button :S [23:10:15] (PS7) Ejegg: QUnit tests for ext.centralNotice.bannerSequence [extensions/CentralNotice] - https://gerrit.wikimedia.org/r/354379 (https://phabricator.wikimedia.org/T144456) (owner: AndyRussG) [23:16:11] (CR) Ejegg: [C: 2] "Pretty comprehensive coverage! The edge case of all steps skipped seems to be the only thing not hit. Clear, well structured tests." [extensions/CentralNotice] - https://gerrit.wikimedia.org/r/354379 (https://phabricator.wikimedia.org/T144456) (owner: AndyRussG) [23:18:50] (Merged) jenkins-bot: QUnit tests for ext.centralNotice.bannerSequence [extensions/CentralNotice] - https://gerrit.wikimedia.org/r/354379 (https://phabricator.wikimedia.org/T144456) (owner: AndyRussG) [23:23:55] ejegg: thanks!!! [23:24:05] thank you for the tests! [23:24:13] they was fun [23:24:25] about to checkou tout the admin ui patch [23:25:38] ah k thx, it's just a wee follow-on [23:25:44] cool [23:26:15] ah, it is a wee one [23:28:52] mmmm heh just a detail.... the "skipToNextStep() return values" does test the case of all tests skipped from the perspective of SequenceManager.... I guess you meant the code path for preBannerHandler, right? Mmm we could add that easily [23:29:01] Sorry I meant all steps skipped [23:29:39] Mmm I also started finally looking at https://gerrit.wikimedia.org/r/#/c/319003/ -- looks good! [23:30:06] (CR) Ejegg: "Looks good. But label 'I' would never be used because the numberOfBuckets has to be a power of 2, right?" [extensions/CentralNotice] - https://gerrit.wikimedia.org/r/355275 (owner: AndyRussG) [23:30:24] Ah heh [23:30:31] that old code is so funny [23:30:38] Yeah README is silly then [23:30:48] Also maybe wrong--where does it get 9? [23:30:57] tinyint(1) ? [23:31:21] oh haha [23:31:33] the (1) is just a visual cue, for column display width [23:31:37] * $wgNoticeNumberOfBuckets: Number of buckets that are provided to choose from-- [23:31:39] this must be a power of two! It must not also be greater than 9 unless a [23:31:42] schema change is performed. Right now this column is tinyint(1) [23:31:43] Default: 4 [23:31:44] it can still store from -128 to 127 [23:32:00] haha [23:32:52] Mmmm well I guess we should pick up some those bits of crumpled insanity next to the park bench while we're out walking the ducklings [23:34:41] (CR) Ejegg: [C: 2] Admin UI: allow all configurable buckets in campaignmanager.js [extensions/CentralNotice] - https://gerrit.wikimedia.org/r/355275 (owner: AndyRussG) [23:34:56] argh big blue button [23:35:00] why do you tempt me so [23:35:06] sorry, removed that [23:35:21] from day 1 i knew that one click c+2 was going to be trouble [23:37:05] hired the same guys who built my workshop to cover it in corrugated metal: https://sadbastard.org/photo/metal.jpg [23:37:12] (CR) Ejegg: "Is there any reason to limit the number of labels? Might as well just use String.fromCharCode('A'.charCodeAt(0) + bucket)" [extensions/CentralNotice] - https://gerrit.wikimedia.org/r/355275 (owner: AndyRussG) [23:37:14] still some detail work to do but it's pretty cool [23:37:34] shiny! [23:38:04] not as shiny as i feared though [23:38:23] thought it was going to be blinding in the sunlight but the ridges seem to mitigate it [23:38:26] good amount of sunlight in there, huh? [23:38:45] yep! south wall [23:39:13] mixed up a batch of vinegar based solution to oxidize it with [23:39:19] but it may not be necessary after all [23:39:35] oh? [23:39:46] didn't realize people intentionally oxidized things [23:40:15] yeah if you want the rusty look you can oxidize it and then clear coat over it [23:40:23] but i may just wait a few years instead [23:40:40] interesting process to watch [23:40:49] totally [23:40:51] guess it depends if you want even weathering [23:41:05] yep [23:41:14] cwd: nice! [23:42:50] (CR) Ejegg: "We check in the .lock because we're sloppily pointing a couple dependencies at dev-master versions. (FIXME)" [extensions/DonationInterface] - https://gerrit.wikimedia.org/r/349092 (https://phabricator.wikimedia.org/T163434) (owner: Gergő Tisza) [23:44:19] ejegg i'm only on for a minute but i'm curious whether in thinking about this it might make sense for donation interface to have a wrapper class for calls to smashpig [23:44:38] mepps: that's a lot of things to wrap! [23:44:50] I'm hoping smashpig can provide all the wrapping itself [23:45:07] the QueueWrapper should be ready for use externally