 This session is a core conversation. It's got two parts, how to get good reviews and how to give good reviews. And the first part is mainly focused at prolific patch writers, but also it's applicable to anybody. And then the second is focus on the people who do reviews. So if that sounds like something that you're really interested in, reviewing core issues, then you're in the right place. Otherwise, you can pick a different session. It's totally okay with me. That's all right. So hi, I'm Kathy. I'm SCT on Drupal.org, and I'm too tall for this microphone. I work for Compress, which is based in Hamburg, Germany. And I work there part-time. They pay me to work on core, which is really cool. And then they pay me to blog about working on core, which is also really cool. And this is Andrea. Hi, I'm Andrea Soeper. I work for The Nerdery, and where I develop for Drupal. And I'm Zen Doodles on Drupal.org. So we have a big problem in that there are a lot of issues, and a huge chunk of them, 20% of them, are needs review. And the problem with having issues lingering in needs review is it's very discouraging for the people who worked on it hard enough to get it to the point that it needs review. It works really hard, and then they cannot progress without somebody else helping them. People who work on core can't work in isolation. The issue queues go back and forth, and it takes many people collaborating together. So if you only have people working on producing a patch, and you don't have people reviewing, it halts the process, and it discourages the people who are making the patches. It's also really hard on reviewers to have so many issues lingering for a really long time at needs review. So there's over 2,000 core issues waiting for reviews. 1,350 have been waiting for reviews for two months or more, and 500 have been waiting for a year. If you contribute and you work really hard and nobody reviews your thing for a year, you're not going to work because you want to be useful with your time. Nobody wants to just work because they work. We want to feel productive and we want to feel useful. Some issues have comments on them, so they kind of got a review, but the review is like, works or doesn't work. And we also want to get better reviews when we do get them. We want more reviews, we want reviews done when the patches are fresh and current, and we want reviews that have actionable feedback. To do this, one of the things we can do is recruit core developers to work on more issues. So some of the things that we're going to talk about is how to make that easier so that we can grow those pools of people. So let's say you are writing a patch and you work really hard on it and you post it in the issue and your market needs review and nobody looks at it. So what can you do to get somebody to look at it? One of the things you can do is in your comment when you upload your patch is you can be really specific about the kind of review that you need because you're going to upload a lot of patches over the life of an issue and initially you might be looking for architectural or proposed solution feedback on the general methodology that you're using. That's a different kind of review than I need a review before we ask a core commander to look at it because that kind of review is I need to make sure that we've met all the core gates that all the standards that we need to are in place. So when you post a patch be really specific about the kind of review you need. Maybe you need an accessibility review. Maybe you need somebody to manually test the thing that you're posting. So just adding some words to direct the people who are going to come to look at it is really helpful. And in addition to adding words to the comment when you post a patch are these great tags that you can tag. And anybody can change the tags on an issue. You don't have to be a special permission on Drupal.org. So use tags. Make sure the issue summary is up to date. Hold on, hang on. Good. I don't have to read all these because I'm going to talk about them. So use tags. Let's go back to that for a minute. So for example, these are some sample tags. The really nice thing about the tags field on Drupal.org is it's auto complete and we've been standardizing the tags a little bit so that when something is needs review they all start with needs. So you can start typing needs space and you get a list of tags that already exist there and you can pick from those. So needs architectural review, needs usability review, needs manual testing, needs screenshot. And this is really awesome because this is how people find your issue who wants to do what you need them to do. So you get people who stumble upon your issue because the status is needs review but they'll be like myself, I'll find a needs review issue but it doesn't need my kind of review. I really like doing manual testing and sometimes that's the thing that I want to do. So I will look for issues with that tag and then do that. So if that's what your issue needs, mark it with the tag and then you will get pools of people who actually want to do that and they will find your issue. Make sure that the issue summary that you have is at least correct. So it doesn't have to be like super perfect. I like issue summaries that use the issue summary template and they're all up to date with all the comments but at least correct any actual misinformation that's in the issue summary. And the reason why that is important is because let's say you really want your patch reviewed, somebody finds your issue because you did use a needs tag and then they start to read the issue summary and they get confused. And then there's 25 comments of an argument and they're a little bit more confused and they get to the end and they see your patch and they're like they want to be efficient. Your potential reviewer doesn't want to waste their time and so they're going to move on to another issue. They're going to be like I'll look for a different issue that needs manual testing that I can understand better. So you're trying to capture these people who find your issue to review your patch and in order to capture them and keep them there so you don't lose them to other issues, you have to make your issue better than the other issues by updating the issue summary. If you don't have time to update the issue summary, that's cool, right? Tag it, Needs Issue Summary Update and somebody else will update your issue summary for you and then you still get people to review it so that's really good. When you're updating the issue summary or in your comment, you've tagged it with what it needs like Needs a Screenshot, Needs Manual Testing, there are some super cool documents already written that are called Contributor Task and if you do a Google search for Drupal Contributor Tasks, you'll find all of these documents already written and the nice thing about that is, for example, there's a Contributor Task document for how to do a review. There's a Contributor Task document for how to add screenshots. There's one for how to do an accessibility review because somebody has found your issue because you used the Needs tags. Somebody is committed to actually doing the review because they can understand the issue summary and what you need them to review about your thing but they don't know how to actually do a good review so that you get the kind of feedback that you want. So if you give them instructions for how to do that review, that means that the review that you get is going to be a lot better but in order, you don't want to waste your time writing out how to give a good review in every issue. That's ridiculous. So there's these Contributor Task documentations that you can post a link to. So if you do a Google search, for example, for Drupal Contributor Task accessibility review, it will give you exactly the link and you can paste that either into the issue summary in the remaining tasks section or just in a recent comment saying, I need an accessibility review. Here's the instructions for how to do an accessibility review. Meta issues are excellent. When you have a meta issue and you have instructions for people how to make all the patches for all the issues that are part of the meta, include a section there which is how to review these patches that are in there because sometimes they're really specific. So take, for example, the schema issues of adding schema for everything in core. The way to review those schema issues is different. Then the way you review all other issues. So having specific instructions in the meta for how to do the review means that the reviews that you get are going to be a lot better. If you need somebody to test something on a mobile device or you need somebody to test something who doesn't have a local dev environment, for example, if you want a blind user to do an accessibility review on your patch, you can build a link with simply test.me. You can build a URL and in that URL, you include the instruction to tell simply test.me to apply your patch to Drupal 8. And then you post that link in your issue. And then from any device and anybody, no matter if they have a dev environment or not, they can review your issue. And it makes it a whole lot easier for people with accessibility issues if they don't have to go through all that process of setting everything up. They just use the simply test.me link. The last part of how to get good reviews on your issues is to give a good review. It educates the general contributor population because they see more examples of good reviews. And then they're like, oh, yeah, that's how we do it in Drupal. And then they give good reviews to you. Also, you can trade good reviews as well. So if another core contributor has an issue that needs reviewed, you could maybe talk nicely and review one of theirs, and then they will review one of yours. So how do we give a good review? I should speed up. Sorry. So the first thing in order to give a good review and to try to maybe facilitate the process of giving a good review is to kind of be tooled up and have all of the tools and access to all of these things to allow you to give a proper review and quickly be able to write all of the important information in one place, including Dread Editor, so that you can just do a review right there in place. Drush, so that if you're going to do any testing, you can do a Drush SQL drop and site install and all of those sorts of things. Simply test.me. As Kathy was discussing, you can apply a patch and test things, which is more of a manual testing sort of thing. And then there's a screen capturing tool to allow you to be able to grab a screenshot and throw it right into the issue quickly. And of course IRC, so that you can collaborate with the other people who may need a review as well. And ask questions, which is one of the things that we talk about here in full-front analysis. So when we're reviewing a patch, it's important to acknowledge effort and encourage the patch submitter by acknowledging their effort. These people have put time and effort and thought into uploading a patch. So even if the patch is completely wrong, we need to make sure that we're still encouraging people to do the right thing by saying thank you for your effort and then supporting improvement by telling them how to make it better. So if they've done it wrong, then to say, you know, it's amazing that you've contributed, it's amazing that you've helped, and here's what I need for you to do to make it better. But on the other hand, if you're not sure about whether or not it is a... But don't just assume it's wrong. Ask questions to maybe validate that maybe their process, their thought process, the way that they've solved the problem isn't a valid way to do it. Ask lots of questions, which goes back to the IRC as a tool for patch review. Does that slide show up? Okay, I just don't have any notes. Okay, I'm sorry. So we need to make sure... The things that we need to check when we're doing a patch review, we need to know does it work? Does it fix the issue? Does it stay on scope? Have we strayed and started changing white space in completely different files or are things missing? Does it introduce rejections? Does it introduce regressions? Does the patch make sense? If you're reading through it, is it readable? Are there enough comments so that the next guy who is reading this code but doesn't have the issue in front of them, will they be able to understand the code? And is there a better way? But this is a really tricky spot, if we remember full formal nicety a minute ago. Is there a better way? Well, sometimes that's important to point out, and sometimes the perfect way is maybe not necessary. Is there a better or more elegant solution? But let's not have fights about core issues just for the purposes of having, you know, fights, right? And then another thing that we need to make sure is these things are blockers. So when we're reviewing, we need to make sure that there is proper documentation that the performance gate is passed. The SQL queries and caches and memory usage. And there's documentation on all of the core gates on Drupal.org and you can search for that and turn that up. Accessibility, like Kathy was talking about, having to do with making sure that a blind person can use a reader and that sort of thing. Usability and testing to make sure that everything has gates. I'm sorry, has a test. And that way we know things don't introduce regressions. Make sure to include a reference to Drupal.org documents when it's appropriate. So if something is missing a core gates sort of thing, then we need to make sure that we reference the document and say, here's what needs to be improved and here's why and here's how to learn how to improve it. Check the previous comments to make sure that everything that's been suggested or noted about the patch has been included. If there are manually testing, make sure that the steps to reproduce match what you've done and if they do not include new steps to reproduce. And if it is something which is being tested in such a way that the browser affects the way something is displayed, then make sure that you include what browsers you tested in so that we know which things are left that need to be tested. And point out anything that's blocking versus things that are non-blocking and consider a follow-up issue for non-blockers. Within your review also use the headings. I'm sorry. When writing your review, use HTML so that it's easy to read and headings and lists, li, embed your images with image tags and use the bracket or hash number in order to generate a link that shows us all of the awesomeness of what the state of the issue is. All right. So if you're looking for the link for the core gates documentation, you can ask in the bot in IRC. You can ask it core gates, and it'll give you the link. So all these things have to work together, right? The people who do reviews need tools so that they can do their reviews efficiently and faster. The people who produce the patches need to do this long to-do list that I gave you to do so that you get people reviewing your stuff and you don't wait two months to get a review. So when we look at that part for the patch producers and we think, how is it faster to do that stuff that Kathy said? Kathy just told me to do all these things. That's more work for me. How is it faster? And there's two parts of that. One is it's not faster for you when you're posting your patch. It means you'll get your review faster. So you won't have to wait two months. You can wait a week to get your review. So that's kind of how that bit is faster. And the other part is that those things that I told you to do are ridiculous, okay? It's stupid that you have to know what tags are there and there's too many to pick from. And you have to kind of already know that you should do that. You need to know that there is such a thing as needs accessibility reviews so that when you start typing it, it auto-completes, right? It's also ridiculous that I'm suggesting that you go Google for a contributor task document on how to do an accessibility review and that that's your burden as the patch producer to include a link to tell reviewers how to review your stuff. It's silly. So I suggest, because core conversation is supposed to be outrageous and suggest things, right? So I suggest that those things in the future are integrated into the way that we use the issue queue so that when you upload a patch, there's checkboxes of what you need and you check off what you need. And it automatically updates in the issue summary the remaining tasks. And if one of the remaining tasks needs accessibility review, then it knows where the instructions to do that are and then in the issue summary, it's right there. So as a patch producer, my ideal vision for you is that you upload your patch and you check off what you need in terms of your review and you hit save. And that's the only thing that you do. And automatically then, the next person who comes and looks at your issue sees that it needs manual testing, it needs accessibility review, it needs docs review, and the instructions for how to do that are already linked there. That's my outrageous suggestion. And that's the end of our talk. So we have nine more minutes for questions on the topic of how to... Or suggestions. Or suggestions. Right, like I want to know your outrageous ideas, because we have this problem. We're not getting reviews fast enough and it's frustrating for everybody. Hi, sorry if you can hear. I can't hear you. Is there a switch? I can hear you, but I don't think that person back there can. I can speak loudly anyway. Get closer to the mic. Hi, I can speak loudly. That's better. So two really quick points. One is that I think that we need a way to make sure that when people do their first patch, that we have a really quick way of making sure they get reviewed. I really don't think that people would mind kind of tagging it themselves to say this is my first patch or this is my early patch and then making sure that we have a priority of jumping on those patches. Another thing, and I haven't kind of worked out in my head how this might work, but the truth of the whole core thing is that people like the credit, people like to have their name on patches and the truth is that we need reviews and we need people to RTBC and we don't get enough of that. And maybe, I know this would be a killer for people that are actually having to put stuff in, but giving people actual credit for reviewing and RTBCing as well on the issue could be a really good way because I think then people would probably troll the issue queues looking for issues to review and actually, you know, it probably is almost as important as having the patches done themselves. There are actually two things that I heard you suggest there and one is giving contributor, sorry, commit credit to reviewers which actually is sometimes done and then the second thing that I heard you say which was right in the beginning which is kind of brilliant but you didn't linger on it very long is that a brand new contributor who is uploading their first patch should have some kind of special flower treatment that this should be something like a tag or something but a brand new contributor who is contributing their very first patch doesn't know to say I am a special flower issue but we already know that because we know this is a new Drupal.org user account and this is your first... Yes, exactly. And I think that's a brilliant idea. We could have a sidebar block. That's why I'm standing here. Yeah. Right. So one... if you're a patch producer and you're an experienced contributor and it was... you know that the person who reviewed your patch worked really hard on it you can suggest a commit credit message in the issue summary because the way that core maintainers sometimes make their commit messages is they use the Dread Editor suggest commit message button and that's what they use because they just want to copy and paste something, right? They want to be productive and if you've been working hard on an issue and you know that somebody is not going to get credit like they're not on that list but you think that they should you can put your own suggestion right in the issue summary and that helps core committers take your advice and give that person... At the very top. At the very top. In a code tag with a big header that says use this as the commit message. The other problem that we have is we used to say things like that we have 0.2% of activeDrupal.org users who are contributors and I've tried to stop saying that what we have is a small... some percentage of people who are patch contributors. We don't have any way of measuring contributors. So yeah, this idea of if it's someone's first patch we can build a view of that we could have a sidebar block that highlights someone's the first attachment that has a patch extension is it too far? You have to get really closer. Okay. This is very... This is like uncomfortably close. I'm not... I just met this microphone. So what we could do is we could have a view that you know... Okay that's funny right because Jess is saying we could have a view and she brought views into core. Okay. We could have a sidebar block that lists attachments with patch extensions for the first time someone has uploaded it to a particular module on that module's project page in the sidebar and we already have... it's actually not that hard to add sidebar blocks. So that's a little bit... that's a bit of a more complicated query but it's doable and it's the kind of thing that might, I don't know it might even be doable before the 777 upgrade because maybe it's not. But in any case I think that would be a great thing to do and I think that you should do that because you suggested it because this is what happens is you suggested an awesome thing but I think it would be amazing and it would be a great tool and that actually gives like for reviewers like looking for someone's first time patch it's like oh I will give this person feedback right away so that maybe the second time they have to wait three months or five years to get a review but the first time they get a response right away from someone who's that's a good idea too right so he's suggesting that it's perhaps the first couple so some small number I think that's a fantastic idea I love it maybe you like have some coloring like this is the first one it's green and then as it okay you visual artistic person over there it's not accessible but I think like it's hard because we're bad it's the same problem for experienced contributors as it is for new contributors right so it's your 100th patch and you don't want to wait two months for your review it's your first patch you don't want to wait two months for your review so sometimes we're phrasing it as what should what do experienced contributors need in order to speed this process up and sometimes we're phrasing it as what do new people but really I think both audiences can really benefit from the solutions that we're talking about hey there Drupal can jump on the badge train that everyone else is on okay and contributors can get you know first five commits or first five patches yeah so in your swag bag which nobody looks at right but okay some people look at we had a post we the core the core mentoring people who are fanatics there's a postcard about core mentoring have a postcard and it has a tag cloud on it and when you see your name on a tag cloud and it wasn't there before you get really excited right so getting a badge and doing gamification is really motivating for some people and I love to play games and the game that I'm playing with myself right now is trying to get to 100 commit credits that's my game right now and that's motivating for me and you know and so I think it's hard because we don't want people to game the system we want to use a gaming system to encourage them to be productive and that's I think why when you suggested game you heard people go oh because we're worried we're worried about that we have to think really hard about it but we we need to do something right I'm one of those people who had really close to the microphone I'm one of those people who had an issue that probably took six or nine months for someone to respond and the issue went for two years but it eventually got in that's alright I can handle that but another idea for review would be people that have reviewed similar types of issues so you could reach out to people that's a very good idea and also when you submit a patch you can take yourself out of the view exclude yourself from the view anyway the other one I had was when you submit an issue it could list other open types of issues that are the same type just a few or something like that but what would be the algorithm if you will for determining what's a similar issue well I guess it kind of hinges on people tagging stuff well but that's one way so now we're back to a manual process I wrote down see a list of people who have reviewed similar issues that was part of what you said and then what was the second bit what time is it? we're at one minute and two hours now the tricky bit about this particular core conversation slot is we had to jam two core conversations into the time for wine despite the fact that Andrea and I could have talked for a lot longer about that and we want to hear your suggestions for a lot we can't so we have to do our next she still has a second she's not you're getting set up right so we have just a little bit a little bit left I have two quick suggestions one is the suggestion for some way of actually tracking once you've submitted a patch if you can actually review somebody else's patch that then bumps your your issue to be reviewed up in a queue or somehow I know that's harder to do I think it's a great idea but it's I mean unfortunately there is no queue to be reviewed really I mean and there's such a it's so I mean I think that in theory it's this trading thing that we do informally but the fact of the matter is that the queue of to be reviewed is there's so many different there's so many patches that I can't review I'm just not capable of reviewing them so it's not like there's any group of people who it's their dedicated responsibility to review patches all across everything because it's it's you know we each have our own skills we each have our own abilities that we bring to an issue and so if I have an understanding of a configuration management issue I can provide meaningful feedback on a patch about that this is Jess web services I can't so sure but I think even just having a time-based listing of this this issue has been sitting in the queue and is completely unreviewed for six months should somehow bump it up the line rather than continuing to let it be it no it's hard I think because sometimes an issue that's been sitting in the queue for six months that hasn't been reviewed is not a priority and if we if we really needed it so we do have ways of prioritizing certain issues the ones that really need to be reviewed this week are the critical ones right because they're critical to moving Drupal forward in general but then you have the problem of you know not just moving the project forward but helping that contributor move forward so it's very tricky so the other question or the other suggestion I have is that while tagging issues is great every issue needs documentation performance testing all the gates that need to be addressed and the few that you could actually say well this is in a contrib issue whatever it may not be relevant it would be much easier to have a flag or whatever else and mark that off that's in the issue that's basically predefined but more by default by default yeah rather than actually that somebody could just say okay these are the five steps that need to make sure that it goes past and it's done or it's not done right and instead of having to actually tag the issues all the time alright I'll write that down okay so we are going to be available after the core conversations so write your question down or tweet your question so you don't forget it right after this is lunch so after our session we can hang out a little bit and make sure that we talk okay can you clap for me us I mean us I feel finished I needed closure thank you well we have to market fix in way two weeks for system message closing