Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 UTC on 18 March 2024, to get $100 off your ticket.
On the dashboard, there's a "Contributor links" block with a variety of links to various queues. It'd be great to add "Novice issues" to that list, to provide new contributors a place to jump in. According to catch, this code lives in drupalorg_project module.
Comment | File | Size | Author |
---|---|---|---|
#32 | novice-link-in-contributor-block-1266380-32.patch | 1.73 KB | Anonymous (not verified) |
#22 | novice-link-in-contributor-block-1266380-22.patch | 5.75 KB | Anonymous (not verified) |
#18 | novice-link-in-contributor-block-1266380-18.patch | 5.75 KB | Anonymous (not verified) |
#14 | novice-link-in-contributor-block-1266380-14.patch | 5.3 KB | webchick |
#13 | novice-link-in-contributor-block-1266380-13.patch | 8.46 KB | webchick |
Comments
Comment #1
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #2
webchickAwesome!! :D So glad to see someone take this on.
If you need any help, don't hesitate to ask, either here or in #drupal-contribute on IRC!
Comment #3
Anonymous (not verified) CreditAttribution: Anonymous commentedThanks for your kind advice webchick.
I've added a link called "Novice issues" under "Issues needing triage".
Code added:
l('Novice issues', 'patch/novice'),
Comment #4
webchickAwesome!! :D I've deployed this on http://issue-summaries-drupal.redesign.devdrupal.org/user/192102/dashboard for testing user/pass: drupal/drupal, then bananas/bananas
In terms of code review, the first thing I was going to flag is that the link title does not have a t() wrapper around it, which is a best practice when creating links. However, the surrounding code shows that none of the other links have this either, so fixing this would be a follow-up issue (perhaps also a Novice one :)).
I also noticed that some of these links are wrapped in drupalorg_project_issue_link(). At first I thought that was unnecessary, but after trying to change /patch/novice to point to project/issues/search?projects=&status[]=1&status[]=13&status[]=8&status[]=14&status[]=15&status[]=2&issue_tag, i realized that's not possible. :( So I think we're going to indeed have to use that method instead.
We also discussed on IRC which statuses to show, and decided on active, needs work, needs review, RTBC, and fixed. Postponed and closed we keep off, so that people don't get confused and think those are actionable.
So needs work for now, sorry about that!
Comment #5
webchickOh. And that bug in path module is at #118072: Allow query strings in URL aliases it looks like.
Comment #6
Anonymous (not verified) CreditAttribution: Anonymous commentedThank you for the quick code review and pointers on which to improve. I have learned a lot again while writing this patch.
I wrapped the link in a new function called drupalorg_generic_issue_link() because the original drupalorg_project_issue_link() would limit the results to the "drupal" project. To get the count for all the issues tagged "Novice" I also added a db_query to the drupalorg_project_issue_counts() function. And I wrapped the link title in a t() wrapper. ;)
Comment #7
webchickWow, great! Thanks for the fast turnaround on this! This is now deployed to the testing site: http://issue-summaries-drupal.redesign.devdrupal.org/user/192102/dashboard user/pass: drupal/drupal, then bananas/bananas
The biggest thing is that the count is not working for some reason. Hm. I'll need to try and debug that later. But in the meantime:
Since you know the actual string you're looking for, this query would be a bit more efficient to use = rather than LIKE. Also note that "Novice" is capitalized.
Actually, I really like what you've done here in creating a re-usable function that could be used for other links that span issues across all projects, not just core. So let's not limit the description just to Novice issues.
Trailing whitespace on this line.
It's best practice to namespace function names by their module name. So I think the proper way to name this function would be "drupalorg_project_generic_issue_link"
However, rather than "generic", how about we go with the word "global".
Then we should probably rename the other function from "drupalorg_project_issue_link" to "drupalorg_project_core_issue_link" so the distinction is clear.
(Sorry, had no idea this would turn into such a sprawling issue! :D)
Hm. For some reason these counts aren't showing up on the sandbox site.
According to the coding standards, we want these commas to have spaces between them. Sorry for nit-picking, but code conforming to coding standards is much easier for other people to pick up and extend later.
Comment #8
webchickAlso, maybe one other thing. We have a bit of a usability issue here where all of the other links in that block, except for "Your issues", point at the core queue, and this one doesn't. I actually like that it doesn't, because I would love there to be a huge pile of Novice issues from all sorts of projects that people can work on. But it breaks expectations.
Further, I also want to promote this initiative a bit, because this is one of the best ways to get new people involved in our community. So how about we move the novice link up closer to the top, right under "Your issues"?
Comment #9
xjmI think that pattern would be more intuitive (and has the advantage of promoting Novice issues as well!).
Comment #10
Anonymous (not verified) CreditAttribution: Anonymous commentedAnd here is the next one! I really appreciate the time and effort everybody is taking to help me progress and learn.
In order to fix the SQL query I have requested access to a copy of the database (http://drupal.org/node/1274630).
The novice link is now placed directly under the "Your issues" link. I totally agree that this is more intuitive.
I fixed the trailing whitespace in both the original I used as an example and in the new function I created.
Both functions are now renamed to drupalorg_project_global_issue_link() and drupalorg_project_core_issue_link() as suggested in #8.
And last but not least I have also fixed the comma after whitespace issue.
The only issue left should in this patch be getting this SQL statement to return the actual count.
btw. I am very happy with your nit-picking webchick!
Comment #11
dwwSince you're linking to a query that restricts to active issues, you should do the same filtering for this count query. I.E. you need
AND pi.sid IN (...)
with the right status ints.Otherwise, looking great. Thanks demarcoz for all your work on this!
Cheers,
-Derek
Comment #12
Anonymous (not verified) CreditAttribution: Anonymous commentedThanks Derek, that is a really excellent observation. I have corrected the SQL statement to include the same filter.
+ $issue_counts['Novice issues'] = db_result(db_query("SELECT COUNT(*) FROM {project_issues} pi INNER JOIN {term_node} tn ON pi.nid = tn.nid INNER JOIN {term_data} td on tn.tid = td.tid WHERE td.name = '%s'", "Novice" AND sid IN (1,2,8,13,14,15)));
I am really enjoying this experience and appreciate all the input. It really motivates me to continue and get more involved. Thanks all!
Comment #13
webchickAwesome! I'm having fun, too! :)
The latest patch had a syntax error in it. Oops. :)
How db_query() works is you write your entire query as you normally would, but stick placeholders in for the various variables, a %d for a number, a '%s' for a string, etc. Then, after that, go the arguments that should go into the placeholders, one after another.
So a query like:
Would become:
And then you'd add the arguments one after another at the end:
Like that.
The problem with the query in your patch is the "Novice" word was kinda stuck in the middle where it visually belonged, but wasn't conforming to the APIs. http://drupal.org/writing-secure-code is definitely worth reading; tons of helpful info there about security functions in Drupal, how to deal with user input text, etc.
So! I fixed that, and lo and behold IT WORKS!!!
http://issue-summaries-drupal.redesign.devdrupal.org/user/192102/dashboard
Apache user/pass: drupal/drupal
Drupal user/pass: bananas/bananas
Here's a fresh patch with the query fix. I think this is ready to go, but would love to see at least one more set of eyeballs on it first.
Comment #14
webchickAhem. :P How about a patch without all the other crap from my sandbox? ;)
Comment #15
webchickHm. No, something is up. The link that it generates is not actually filtering the issues to novice ones. :\
Link generated by drupalorg_project_global_issue_link:
http://issue-summaries-drupal.redesign.devdrupal.org/project/issues/sear...
Correct link:
http://issue-summaries-drupal.redesign.devdrupal.org/project/issues/sear...
Hm.
Comment #16
webchickThis link also works:
http://issue-summaries-drupal.redesign.devdrupal.org/project/issues/sear...
Unless I'm mistaken, the difference between this and the link generated by the module is the order of statuses. WTF?
Ok, time for bed. ;)
Comment #17
webchickOH. I see the problem.
Compare: issue_tags_op=or&issue_tags=Novice (working)
with: issue_tags_op[0]=or&issue_tags[0]=Novice (not working)
So those arguments need to come in as strings vs. arrays. Well, that sounds like a fun little puzzle to solve! ;) Back to you, demarcoz! :D
Comment #18
Anonymous (not verified) CreditAttribution: Anonymous commentedWow, thanks for your feedback and help. I understand now where I went wrong with the SQL syntax. Thanks for explaining.
I had a little trouble applying your latest patch into my own git repository. So I replicated the SQL query change manually and changed the 'or' and 'Novice' values to be strings instead of arrays.
Review time again! ;)
Comment #19
dwwNice. However, now that you've learned how our DB query API works to prevent SQL injection with user-supplied data, I feel compelled to point out that nothing in here is user-supplied. ;)
If the string "Novice" was actually user-supplied (i.e. there was a text box when configuring this dashboard block or something) and stored in a variable like $issue_tag, this would be exactly the right thing to do. However, since it's hard-coded, it's kind of pointless to write the query this way. Furthermore, if there was some good reason to do this for "Novice", you should also do it for all the int values for sid.
But, I'd say neither makes sense to use placeholders for. The placeholder just makes the query a little harder to read and understand, and slightly wastes computation we don't need to be doing. So, you should just have this:
And while that probably works, there's another possible problem in this query... "sid" is potentially ambiguous. In this case, there's only one table being JOINed that has that column name, so you're fine, but it's an extremely good practice to always specify a table for columns like this.
So, what you want looks more like this:
(note the
pi.sid
in there).However, thanks to our pedantic friend the coding standards, that's not quite right, either. ;) What you really want has spaces after the commas (as webchick pointed out above), like so:
Make sense?
Thanks,
-Derek
Comment #20
webchickOh, my advice was pwned by dww. Nice! :)
Comment #21
webchickSandbox updated with the patch in #18, plus dww's suggestion in #19.
The one thing I notice is that the counts are *way* off. It says there are 486 issues, but there's less than 50 when you click the link. However, I'm pretty sure that's just a factor of how the sandboxes are set up; they clear data from node to keep the data size down, but I don't think they clean it up in term_node, project_issue, and so on.
So, if we get one last patch that's #18 and #19 combined, I think we can get this sucker deployed. YAY! :D
Comment #22
Anonymous (not verified) CreditAttribution: Anonymous commentedThanks for pointing out the nuances in this Derek! I have prefixed the "sid" column with the table name and replaced the placeholders with the actual values. Last but not least every comma is now directly followed by a whitespace character. Basically a combination of #18 and #19 as webchick suggested.
I'm glad we had this back-and-forth it really makes the patch process and interaction clear. ;)
Comment #23
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #24
webchickPerfect!! Thanks so much, demarcoz!
I've deployed that to the sandbox, and all seems to be working.
Marking RTBC and tagging for deployment. YEAH!
One quick note about deploying, I had to clear caches on the sandbox before the link/count would show up.
Comment #25
Anonymous (not verified) CreditAttribution: Anonymous commentedVery cool. Maybe I should change my filters to only show issues from webchick. ;)
Thanks a lot everybody, I learned a lot and I will definitely keep working on the issues.
Comment #26
drummCommitted and deployed. I'll let the cache clearing run its course to get the counts updated.
Comment #27
webchickYES!!! Thank you Neil!! GREAT job, demarcoz! Hope to see more patch contributions from you in the future. :D
Comment #28
tsvenson CreditAttribution: tsvenson commentedI just created #1278500: New "Novice issues" link in Contributors links looks like its only core issues about reorganizing the contributors links block since I took it as the Novice link was just for core issues.
Comment #29
SeanBannister CreditAttribution: SeanBannister commentedI noticed the link also displays fixed issues where the other links in that block only display currently active issues which I feel makes more sense as a fixed issue doesn't need to be looked at.
Comment #30
dwwWhoops, I didn't even notice that when reviewing (and never actually tested). Yes, I agree that fixed novice issues aren't interesting to count nor view, so we should be excluding those. That means removing "2" from those lists of status ints in both places in the previous patch.
Also, the change was already deployed so we should have removed that tag when that happened. We can re-add that tag once this follow-up fix has been committed and is ready to deploy.
Thanks all,
-Derek
Comment #31
webchickdemarcoz thought that fixed issues were useful, so that new novices could see what other novices had done. I tend to agree.
Comment #32
Anonymous (not verified) CreditAttribution: Anonymous commentedAs webchick already mentioned I found it useful being able to reference various issues in different states. But maybe that's a tip best mentioned in the documentation.
I have removed the "fixed" state from both the query and filter.
thanks,
Marco
Comment #33
dwwThanks. Reviewed, committed, and deployed.
Comment #34.0
(not verified) CreditAttribution: commentedx