[06:34:59] https://bug-attachment.wikimedia.org/attachment.cgi?id=16760 [13:00:23] (PS1) Ssmith: Merge branch 'master' of ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/DonationInterface into HEAD [extensions/DonationInterface] - https://gerrit.wikimedia.org/r/166564 [13:02:12] (PS1) Ssmith: Merge branch 'master' of ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/DonationInterface into HEAD [extensions/DonationInterface] - https://gerrit.wikimedia.org/r/166565 [13:04:13] (PS1) Ssmith: Remove name@email.com from WP form [extensions/DonationInterface] - https://gerrit.wikimedia.org/r/166566 [13:05:07] (Abandoned) Ssmith: Merge branch 'master' of ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/DonationInterface into HEAD [extensions/DonationInterface] - https://gerrit.wikimedia.org/r/166564 (owner: Ssmith) [13:13:14] (PS1) Ssmith: Update legal text on landing page [extensions/DonationInterface] - https://gerrit.wikimedia.org/r/166567 [13:13:48] (CR) jenkins-bot: [V: -1] Update legal text on landing page [extensions/DonationInterface] - https://gerrit.wikimedia.org/r/166567 (owner: Ssmith) [14:36:39] (PS1) Ejegg: Don't retry on certain errors [extensions/DonationInterface] - https://gerrit.wikimedia.org/r/166573 [15:02:18] Morning ejegg! :) [15:02:29] Hi AndyRussG ! [15:02:39] How's it going? [15:03:01] Not bad, plugging away at stuff... [15:03:20] I was trying to test your patch for sending new info to banner impressions... [15:03:28] Oh yeah? [15:03:36] How'd that do? [15:04:01] well I haven't gotten to the patch itself ...I found that my subscribing CentralNotice borked because of an incompatible update [15:04:13] https://www.mail-archive.com/wikitech-l%40lists.wikimedia.org/msg78108.html [15:04:27] Removel of jquery.json [15:04:38] ohhh, right [15:04:50] They went from deprecating that to removing it pretty quickly [15:04:51] On my local wiki it actually doesn't even load the bannercontroller because it complains of the missing module [15:04:58] Yeah hit 'n' run [15:05:19] On production it still servers banners, thankfully [15:05:48] I guess it must be some production thing where it doesn't die if there's a dependency missing [15:05:53] yeah, it's not actually running on any of the sites we've got up on pre-1.23 versions [15:05:55] Or was the code updated? [15:06:21] ? [15:06:38] https://en.wikipedia.org/wiki/Special:Version [15:06:51] oh wait, other way around [15:07:08] jquery.json shouldn't be in the dependencies anymore [15:07:13] let me see which patch that was [15:07:37] did you branch off master a long time ago? [15:08:38] Looks like september 4th it got merged [15:09:00] Phew, yes https://gerrit.wikimedia.org/r/#/c/158103/ [15:09:28] Yeah didn't know that was fixed, just ran across the missing dependency 8p [15:09:51] Sorry I missed that one ;p [15:10:44] No worries! [15:10:54] Thanks for testing it [15:11:12] np, heh I'll get to testing the actual patch in a sec then [15:34:04] (CR) AndyRussG: [C: 2] "Great! :) Works as expected..." [extensions/CentralNotice] - https://gerrit.wikimedia.org/r/165607 (owner: Ejegg) [15:34:21] (Merged) jenkins-bot: Give RecordImpression different reasons for different cases [extensions/CentralNotice] - https://gerrit.wikimedia.org/r/165607 (owner: Ejegg) [15:59:38] (PS2) Ejegg: Don't retry on certain errors [extensions/DonationInterface] - https://gerrit.wikimedia.org/r/166573 [16:17:42] (PS1) Ejegg: Comment clarifications and formatting [extensions/DonationInterface] - https://gerrit.wikimedia.org/r/166594 [16:17:45] (CR) jenkins-bot: [V: -1] Comment clarifications and formatting [extensions/DonationInterface] - https://gerrit.wikimedia.org/r/166594 (owner: Ejegg) [16:20:01] (CR) AndyRussG: "Works great! Apologies for seeking ever-smaller nits..." (2 comments) [extensions/CentralNotice] - https://gerrit.wikimedia.org/r/166138 (owner: Ejegg) [16:24:41] (PS2) Ejegg: Comment clarifications and formatting [extensions/DonationInterface] - https://gerrit.wikimedia.org/r/166594 [16:29:22] (PS2) Ejegg: Clarifications: rename one var, delete another [extensions/CentralNotice] - https://gerrit.wikimedia.org/r/166138 [16:30:21] (CR) Ejegg: "Suggestions implemented!" (2 comments) [extensions/CentralNotice] - https://gerrit.wikimedia.org/r/166138 (owner: Ejegg) [16:32:06] (CR) Ejegg: [C: 2] Remove name@email.com from WP form [extensions/DonationInterface] - https://gerrit.wikimedia.org/r/166566 (owner: Ssmith) [16:38:27] (CR) AndyRussG: [C: 2] "Woohoo! :)" [extensions/CentralNotice] - https://gerrit.wikimedia.org/r/166138 (owner: Ejegg) [16:38:38] (Merged) jenkins-bot: Clarifications: rename one var, delete another [extensions/CentralNotice] - https://gerrit.wikimedia.org/r/166138 (owner: Ejegg) [16:53:17] ejegg: Good morning! I've only got a couple minutes before I have standup, but: How's the MC retry-avoision thing goin? [16:53:20] *going. [16:53:40] I've got one patch up for review that avoids the immediate retries [16:53:59] Okay, cool. [16:54:15] and another that adds a 'force_cancel' key to the transaction result, but not sure how necessary that is. Will put it up for review anyway [16:54:24] looking at worldpay now [16:54:25] Hmm. [16:54:27] That's interesting. [16:54:44] Here's a thought. [16:54:58] yeah, just a way to pass a message up the stack to where we add the antimessage [16:55:13] Do we... punish people in the velocity filters for trying having a card that is sufficiently toxic to earn one of those? [16:55:18] :D [16:55:52] so, using the 'action' after all? [16:56:01] Not actually, no. [16:56:25] Action only gets used if the antifraud filters are run. [16:56:34] We'd just be stuffing for the next attempt. [16:56:43] Oh, but it would be a proxy way to make sure we're not initiating a new charge attempt on the bad card. right [16:56:50] ...in case they have a card in their master theft spreadsheet that *would* go through. [16:57:08] Heh... nearly. :) [16:57:25] good thought, I'll try to make that happen [16:57:40] It should be relatively... simpleish. [16:57:59] There may not be a nice neat static function in the velocity filter... yet. [16:58:07] And that's okay. Just add one. [16:58:31] the filters see the whole txn response, right? [16:58:42] nvm [16:58:53] Though, it might involve a little bit of a headstand, because none of the filters are installed by default, so you'll have to hook in to it, and dangit I'm going to be late. [17:02:20] I guess we don't get awight today? [17:03:10] bet not, seeing as it's Tuesday [17:06:07] Hmm OK thanks [17:07:42] I have some additional bucketdoc and bucketthoughts to run by everyone, found a possible problem but I thought Adam would be best placed to know quickly if it's real or just a ghost in the machine :) [17:08:31] Here's the unproblematic bit, just rewrote and added some less technical details: https://meta.wikimedia.org/wiki/Help:CentralNotice#Targeting_with_Buckets [17:08:49] Here's the new more technical overview: https://www.mediawiki.org/wiki/Extension:CentralNotice/Buckets [17:09:18] And this is the issue that I'm not sure exists: https://www.mediawiki.org/wiki/Extension:CentralNotice/Buckets#Cycling_bucket_cookie_expiries.3F [17:09:35] (in case you or anyone else feels like taking a peek) [17:10:02] definitely! I'll take a look at that in a few min. [17:10:23] thanks much! [17:11:12] K4|meeting pizzzacat ^ (in case you're interested) (also Hi BTW) [17:36:34] K4-713: ding 'em on both session and IP velocity, right? [17:37:27] ejegg: Oh hey. Actually, just the IP should be sufficient... but it might be cool if there's a different TTL for external reasons. [17:37:45] Ah, ok. Session is way easier! [17:37:54] But bots don't really do sessions [17:38:07] Yep. [17:38:19] So, the same way we use globals to configure everything, should set the ttl for toxic card attempts. [17:45:16] (PS1) Ejegg: WIP: Another safeguard around MC-forbidden retries [extensions/DonationInterface] - https://gerrit.wikimedia.org/r/166607 [17:56:30] ejegg: So, one comment so far. Instead of using setTransactionResult, you should probably do something more final and finalizeInternalStatus. [18:05:36] https://www.mediawiki.org/wiki/Extension:CentralNotice/Buckets#Cycling_bucket_cookie_expiries.3F [18:17:57] meganhernandez: Check your email. CC is fine until at least Thursday evening. [18:18:26] K4-713: finalizeInternalStatus down in processResults, then check getFinalStatus() back up in transaction_CreditCard to set the cancelflag and add_antimessage? [18:18:36] * K4-713 blinks [18:18:45] That statement is going to take me a minute to parse. [18:19:07] sorry, regarding your comment earlier [18:19:11] Yeah, I get that. [18:19:18] It's the... weaving. [18:19:32] Hard to follow when task switching. [18:19:40] sorry [18:19:50] So, a bunch of things should be checking getFinalStatus() in the parent class already. [18:20:08] Oh right, let me see [18:20:56] Guh. This ends up touching some stuff that I have been wanting to fix for a while. [18:21:09] ...namely, that we are trying to send messages to ourselves for failed transactions. [18:21:12] Which is lame. [18:21:19] We don't want that garbage data. [18:21:39] We're already sending something to fregde whenever we finalizeInternalStatus. [18:21:43] That's all we need. [18:23:50] ejegg: Can you add links to your related commits to 1887? [18:24:01] will do [18:24:06] Thanks. :) [18:25:51] OK, I see DoStompTransaction checking the FinalStatus [18:27:23] ejegg: Gah. This whole thing just makes me really sad. [18:27:39] There was a time in which there were clear lines about when things were supposed to happen. [18:27:52] And then people just hacked on it for a couple years, to get things done ASAP. [18:27:57] ...we need a code defrag. [18:29:00] yarp [18:29:28] I promise it didn't used to be this much of a hairball. :( [18:30:31] Ah, the other thing that would be neat... would be if you can roll some log checking in to your tests, after you finalize internal. [18:31:16] ...even better, there's a place to include an arbitrary status code in that log. All that stuff makes it to fregde. [18:31:45] That way, we can look at it later. [18:31:51] By code. [18:31:54] Seems useful. [18:32:38] So I'm just looking at the path from transaction_Confirm_CreditCard through do_transaction( 'GET_ORDERSTATUS' ) through do_transaction_internal through processResponse and back up, and I don't see where anything checks the finalStatus before transaction_ConfirmCreditCard continues along... [18:32:58] No parent class stuff? [18:33:32] Well, that's stupid. [18:33:40] do_transaction and do_trans_internal are both parent class, but thjey aren't checking the final status before returning the txn results [18:35:26] * K4-713 sighs [18:36:13] So maybe I'll keep the force_cancel flag, and just add finalizeInternalStatus up in txn_confirmCC where I check for force_cancel? [18:36:35] Wait 1 [18:36:36] since it already does finalizeInternal for a couple other cases [18:36:37] ok [18:37:16] Hurm. [18:37:55] Oh, crud. [18:38:21] There is a problem with adding another, more-failed status to the list of what we already have. [18:38:32] A ton of things check for the final status being "failed". [18:38:37] oh? [18:38:40] Not... ultrafailed or whatever. [18:38:59] Yeah... check out just about anything that serves up a Thank You or error page. [18:39:21] Yeah, I was going to finalize to failed, but leave the extra force_cancel key in transaction_results just to pass the message up the stack [18:39:27] That makes sense. [18:39:35] Are you testing for an antimessage being sent? [18:39:53] No, guess I should though [18:39:57] Also... did you try to do anything with batch mode? [18:40:09] The place I am most worried about all of this, is the orphan slayer. [18:40:30] ...which probably means I should write that part. [18:40:42] Or, at least, the tests that prove we need more there. [18:40:56] txn_ConfirmCC is called by the slayer, though [18:41:04] so fixing it there should do both, no? [18:41:05] yep [18:41:19] I mean, we should definitely check for that. [18:41:46] Yeah, I'll add something to the orphan tests too then [18:41:51] cool. [18:42:38] At some point, I'll probably go mad and refactor all the places we daisy-chain requests together, to stop trying if we have already finalized internal. [18:43:00] Move to the parent do_transaction instead of between all the things. [18:43:02] oh, please do! [18:43:13] I don't.... know why it's not in there like that now. [18:45:37] does anyone know who the master of WMF mingle is [18:45:37] ? [18:45:59] You. [18:46:02] Congratulations. [18:46:04] har har har [18:46:05] jerk [18:46:06] :P [18:46:43] Ah, but more realistically... awjr probably knows, but he doesn't hang out here anymore. I'll ask. [18:46:46] well in that case, i can indescrimately delete accounts? [18:46:51] i'll ping him, i've got a thread with him anyway :( [18:46:53] oops [18:46:54] :) [18:50:15] I hate it when I use the wrong face. [18:52:49] every mornign it's a struggle [19:01:38] food? anyone? [19:05:05] atgo: Yes. [19:05:10] K4-713: Good call on the antimessage test - it doesn't add them for orphans, even if add_antimessage is set. [19:05:27] ejegg: Wait, really? [19:05:33] You do your food thing, I'll chase this down [19:05:38] Oh, but wait. [19:05:47] If they're actual orphans, we don't want an antimessage. [19:05:55] ah, of course [19:06:00] we're just acking the real message [19:06:17] Er, yes. [19:06:20] Basically. [19:06:35] Everything that doesn't have a message and and antimessage will try to get reprocessed. [19:07:09] So, message send on the way out, antimessage send on return... but only if the person coming back checks out. [19:07:34] I'm going to go eat something before I forget. [19:56:06] (PS2) Ejegg: WIP: Another safeguard around MC-forbidden retries [extensions/DonationInterface] - https://gerrit.wikimedia.org/r/166607 [20:11:55] ejegg: Hey, I just noticed something. [20:12:25] One of the codes is a simple invalid card number? [20:12:48] Actually... arguably two. [20:13:06] yeah? [20:13:27] Oh dan, we don't want to go overboard on those guys then [20:13:31] *dang [20:13:37] We probably shouldn't punish 301, 323, and 305 with IP filtering. [20:14:20] OK, I need to rebase that one anyway. [20:14:25] But all the ones that are like "Take the card from the person and don't give it back" should probably be jerks about it. [20:14:38] yeah [20:25:12] K4-713: was the custom error code mechanism you mentioned adding an 'errors' array of 'internal-000x' => 'some message' to the transaction_result? [20:25:30] ejegg: Oh, that thing. [20:25:57] It's, ah... near the error code translation stuff. [20:26:29] Because we usually can't yell at donors in all-caps half-English about what happened with their transactions... [20:26:52] Usually. [20:27:11] ejegg: defineErrorMap() is the function. [20:27:38] Yep, got that. But hmm, if I add that down in processResponse, it never adds an anti-message. [20:27:59] Hurm. [20:28:02] Does it get that far? [20:28:09] Or, does it bail out earlier? [20:28:19] ...because if there was an error back down there, it shouldn't send any limbo message? - think it must bail out earler [20:29:26] Well, hitting an error like that doesn't necessarily make everything stop. [20:29:54] Are you working on a particular patch I can look at? [20:30:49] I'll just update that WIP in review, one sec [20:30:49] cool. [20:32:07] (PS3) Ejegg: WIP: Another safeguard around MC-forbidden retries [extensions/DonationInterface] - https://gerrit.wikimedia.org/r/166607 [20:32:22] (CR) jenkins-bot: [V: -1] WIP: Another safeguard around MC-forbidden retries [extensions/DonationInterface] - https://gerrit.wikimedia.org/r/166607 (owner: Ejegg) [20:32:54] it makes the tests fail 'cause they're looking for antimessages [20:33:06] Good, good. [20:33:32] So, one possibility is that we're just... not sending one now. [20:34:15] * K4-713 goes to look at logs [20:35:33] ...what the... [20:35:37] ehh? [20:37:46] These logs look... funny. [20:38:30] But, the slayer is running. [20:39:11] I'm not seeing any logged xml being sent/received from the box that runs the slayer. [20:39:32] oh fooey [20:39:37] that's no good! [20:39:42] Lots of crap about utm_source, but no... [20:39:47] ...get_status. [20:40:54] Curious. [20:41:28] Going to the archives... [20:43:23] September 18th was the last time it tried. [20:43:35] whaaa? [20:43:45] * K4-713 frowns [20:44:27] I wonder if this has anything to do with... [20:45:10] ARGH MINGLE. [20:45:19] when did we deploy my cvvresult stuff, i wonder? [20:45:31] Why does it have to say things like "25 days ago" instead of... something useful? [20:45:47] Who exactly wants "25 days ago" instead of... [20:47:28] Okay. Actually this has more to do with awight changing staging to the new version of mw core. [20:47:35] Probably. [20:47:51] 25 days ago is suspiciously close to the 18th. :p [20:48:35] Well, that's interesting. [20:54:02] ejegg: Well, email sent about that. [20:54:05] Hurm. [20:54:32] Now I've completely forgotten what the initial question was. [20:54:46] heh [20:55:06] whether we need to send anti-messages at all if we error the thing out in processMessage, I think [20:55:15] I mean in processResponse [20:55:37] So, here's the deal. [20:55:42] Or, at least: The intended deal. [20:55:50] * K4-713 takes a deep breath [20:56:22] Globalcollect. The first bit of communication we have with their servers, is the user fetching an iframe. [20:56:49] oh, dur, right, there will always be a message there from when we got the iframe url [20:56:51] For all intents and purposes, they have left our servers when they load the 3rd party iframe content to fill out their credit card information. [20:57:19] The point at which they request the iframe (and haven't filled out any credit card information yet), we add a limbo message to the cc-limbo queue. [20:57:28] That just means that this is a person we expect to come back. [20:59:26] Then, they fill out some information at GC, and come back to us. We check to see if they are who they're supposed to be (to be okay to finish off this transaction), and then interrogate GC about what happened. [21:00:01] With Confirm_CreditCard, right? [21:00:09] which calls GET_ORDERSTATUS [21:00:11] Because for some daffy reason, they don't just tell us in the querystring when people come back to us... even though they do tell us how some other things went. [21:00:51] right, like CVV and AVS results [21:01:12] Yeah... which we don't trust anymore anyway. :p [21:01:12] So basically... no matter what happens at GET_ORDERSTATUS... [21:02:17] We should probably be sending the antimessage the second it returns with anything other than "Sorry: GC has temporarily exploded." [21:03:41] * K4-713 double-checks [21:04:15] Right. Looks like we don't when the status is still 'pending' either [21:04:31] That probably seemed like a good idea at the time. [21:04:37] I'm not so sure anymore. [21:06:49] ejegg: What do you think about moving the antimessage sending to a place that's... probably more genuine than where it is now? [21:06:59] I mean, all we really want to know is that they came back. [21:07:09] Making sure that GC didn't break in the last half second is just a bonus. [21:07:29] And, it would simplify the existing hairball. [21:08:28] That seems like a good idea [21:08:49] But even after staring at this stuff for a week I'm not sure where the most straightforward place would be... [21:09:05] Back up in the resultswitcher? [21:09:18] brr... nah, not that far, probably. [21:09:32] I'd maybe do it first thing for everything in the cc confirm mess. [21:09:46] ...and then remove it from pretty much everywhere else. [21:09:51] so as soon as we know whether it's an orphan or not [21:09:59] Actually... no. [21:10:10] And by "no" I probably mean "yes". [21:10:14] heh [21:10:19] (argh it burns) [21:10:36] glad it's not just me reduced to confusion and indecision over this code [21:10:38] Just to double check: You mean if it's not an orphan, send the message, right? [21:11:12] right [21:11:20] Okay. [21:11:21] Yes. [21:11:22] Perfect. [21:11:31] You should see the stupid drawing I had to do for this terrible function on the first pass. And that was before orphans were a thing. [21:11:37] eesh [21:25:20] OK, looks like we /do/ add antimessages for orphans in txn_confirm_cc when we fraud-fail them [21:29:23] Yeah, I know that's a thing. [21:29:36] ejegg: I remember that being an issue at one point. [21:30:55] Even though the fraud filters would fire in batch mode, there would be different velocity-related results. [21:31:12] So, the orphan slayer would be more... forgiving. [21:31:34] makes sense [21:31:42] But, we should just send one all the time if we're even running that function as a non-orphan. [21:32:15] Right. But for orphans, only send an antimessage on fraud fail [21:32:35] oh wait, or completion [21:32:36] Orphans shouldn't have to send antimessages at all. [21:32:41] oh, ok [21:33:40] The only function that antimessages serve is to... reduce the list of everybody that left, to everybody that also didn't come back. Only after that do we run the remainder of the queue through the batch mode and try to pick them up again. [21:34:09] Happily, nothing actually cares if there are too many antimessages. [21:34:17] whew [21:34:21] It just blows all of the matches up, no matter how many there are. [21:34:25] :D [21:34:58] ejegg: Did you get my awesome drawing? [21:35:24] I kept, like, two pieces of paper from 2011. That was one of them. [21:35:31] ooh, let me check my mail! [21:36:45] got it, thanks! [21:37:10] heh, nice [21:37:23] Hopefully, this doesn't tell you anything. [21:37:33] I mean, it's the scribblings of a crazy person. [21:37:43] worth a giggle anyway! [21:37:51] (well, other than "K4 likes TRON", but that's pretty subtle) [21:38:13] oh snap, what do they call that thing? [21:38:24] I knew I recognized that shape [21:38:25] Recognizer. [21:38:28] nice [21:38:30] pfffhaha [21:40:05] ohhhh, rectifyOrphan needs some specific results to decide it's rectified and put it in the ack bucket... [21:41:10] ejegg: So, let [21:41:13] me see... [21:41:18] ...if I can remember all the things. [21:41:34] It doesn't take everything for a couple really good reasons. [21:41:47] The best reason is that it takes people time to fill out their credit card information. [21:42:05] So, we don't even check messages that are less than... 30 minutes old or something. [21:42:44] IIRC, it was a minute or something longer than the Globalcollect iframes live after they're requested. [21:42:49] right, those don't even get to the rectify function I htink [21:43:13] Yeah, it shouldn't even touch fresh messages. [21:43:28] But in the rectify function, it either has to look successful or have one of a few specific statuses to return 'true' and go into the ack bucket [21:43:36] And then... well, this piece of code behaves *very* differently under heavy load. [21:43:47] Oh, that one. [21:45:01] Yeah, that could probably use some cleanup too. [21:46:03] I mean, we would rather not try to process a batch when GC explodes or times out a bunch. But other than that, the alternative is that they get stuck in some pending state and we retry them every five minutes until somebody notices. [21:46:15] ...clearly not ideal. [21:46:34] This is why this is the piece of code I was most worried about when "penalties for retrying" came up. [21:54:06] (PS4) Ejegg: Another safeguard around MC-forbidden retries [extensions/DonationInterface] - https://gerrit.wikimedia.org/r/166607 [21:54:08] (PS1) Ejegg: WIP: Penalize toxic card IPs in velocity filter [extensions/DonationInterface] - https://gerrit.wikimedia.org/r/166678 [22:03:27] ejegg: Hey, got your latest patches. [22:03:37] What are you up to right now, while I'm reviewing these things? [22:04:39] pizzzacat had some relatively small changes they wanted deployed before... tomorrow. [22:05:15] oh, just trying to write orphan tests [22:05:30] I reviewed one of pizzzacat's patches, but the other is failing some tests. [22:05:36] I can fix it up, probably [22:06:35] oh, something in the test seems to be barfing on unescaped ampersands in URLs. Which I think we totally don't want escaped anyway [22:06:44] so, test is broken, not patch [22:06:46] ejegg: Which patch is causing the issue? [22:07:10] 'Update legal text on landing page' [22:07:21] Ah. [22:07:40] Yes, that's... one of them, all right. [22:08:15] oops, the other I just reviewed. submitting... [22:08:30] But, if it's the test I'm thinking of, it might be barfing on purpose. [22:08:32] Sort of. [22:08:37] (ew) [22:09:12] bahaha, I just noticed your topic name. [22:09:29] MC Repeat is your new rapper name. [22:09:47] can't it be my highland clan? [22:10:02] ...yes. [22:10:04] McRepeat [22:10:05] How has this category of jokes never come up before? [22:10:10] right? [22:10:12] Either category, really. [22:10:21] I bet I could be MC BIN Tables [22:10:34] heh [22:10:43] urg, merge mess [22:11:15] (PS2) Ejegg: Remove name@email.com from WP form [extensions/DonationInterface] - https://gerrit.wikimedia.org/r/166566 (owner: Ssmith) [22:12:00] (CR) Ejegg: [C: 2] "Rebased" [extensions/DonationInterface] - https://gerrit.wikimedia.org/r/166566 (owner: Ssmith) [22:12:01] ejegg: So, you currently have three patches out for the GC fixes? Am I missing anything? [22:12:31] https://gerrit.wikimedia.org/r/#/c/166573/2, https://gerrit.wikimedia.org/r/#/c/166607/, and https://gerrit.wikimedia.org/r/#/c/166678/ [22:12:33] (Merged) jenkins-bot: Remove name@email.com from WP form [extensions/DonationInterface] - https://gerrit.wikimedia.org/r/166566 (owner: Ssmith) [22:12:41] yeah, those 3 [22:12:50] Okay! I'm on it. [22:13:15] Will have another for orphan tests soon, I hope [22:13:27] hey AndyRussG could you add card(s) for the documentation work you did? :) [22:13:42] throw them in the backlog and then pass them through to deployed (i know, it's silly) [22:17:53] atgo_: OK u bet :) [22:18:00] thanks~ [22:18:39] np :) did think about it, wasn't sure if I should [22:33:39] (CR) Katie Horn: "Looks mostly good! Many inline comments." (7 comments) [extensions/DonationInterface] - https://gerrit.wikimedia.org/r/166573 (owner: Ejegg) [22:35:35] K4-713: The duplicate order id in the errors array is there to test whether it retries immediately with that retry_vars loop [22:36:22] Hmmm. [22:36:45] I find it suspicious that this works, then. [22:36:47] If you have the dupe order id, it sets the retry vars to non-null [22:36:48] :/ [22:37:07] Yeah, and then it should immediately retry, right? [22:37:22] Well, it looks at all the other errors first [22:37:43] So I had the toxic conditions null out the retry vars and return immediately [22:37:47] So, there are a couple confusing things about having double-errors in the test xml. [22:37:55] Mostly, I've never seen it in the wild. [22:37:57] Worth a comment, at least [22:38:00] Oh, ok [22:38:11] So might be testing for a condition that never comes up [22:43:54] Could be, yeah. Which doesn't mean it won't. [22:44:28] oh dang, I should have set retryvars to null there, not an empty array. I did that in the next commit, I think [22:44:34] ejegg: Hey: Do you have the most recently updated list of self-review patches in core? [22:44:58] And, maybe a brief explanation of what you did to get it? [22:45:23] yeah, that list attached to the email thread should be up to date, as it was just covering 1.22->1.23. I think the log command and the awk script are also there. [22:45:34] I can resend just that [22:46:01] Awesome. [22:49:34] Oops, didn't change the subject line when I forwarded it, so it'll probably just stick on to the same conversation. Hopefully it bubbled to the top of your inbox for the moment [22:49:48]