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.

Comments

Anonymous’s picture

Assigned: Unassigned »
webchick’s picture

Awesome!! :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!

Anonymous’s picture

Status: Active » Needs review
FileSize
432 bytes

Thanks for your kind advice webchick.

I've added a link called "Novice issues" under "Issues needing triage".

Code added:
l('Novice issues', 'patch/novice'),

webchick’s picture

Status: Needs review » Needs work

Awesome!! :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!

webchick’s picture

Oh. And that bug in path module is at #118072: Allow query strings in URL aliases it looks like.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
2.86 KB

Thank 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. ;)

webchick’s picture

Status: Needs review » Needs work

Wow, 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:

+++ b/drupalorg_project/drupalorg_project.module
@@ -200,6 +200,7 @@ function drupalorg_project_issue_counts() {
+  $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 LIKE '%s'", "novice"));

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.

+++ b/drupalorg_project/drupalorg_project.module
@@ -536,6 +537,24 @@ function drupalorg_project_issue_link($text, $query = array(), $absolute = FALSE
+ * Project issue link generator for all novice issues.

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.

+++ b/drupalorg_project/drupalorg_project.module
@@ -536,6 +537,24 @@ function drupalorg_project_issue_link($text, $query = array(), $absolute = FALSE
+ *   A keyed array of options to pass to the query parameter of l(). ¶

Trailing whitespace on this line.

+++ b/drupalorg_project/drupalorg_project.module
@@ -536,6 +537,24 @@ function drupalorg_project_issue_link($text, $query = array(), $absolute = FALSE
+function drupalorg_generic_issue_link($text, $query = array(), $absolute = FALSE) {

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)

+++ b/drupalorg_project/drupalorg_project.module
@@ -841,6 +861,14 @@ function drupalorg_project_bingo_block_output() {
+          $counts_novice . t(' Novice issues'),

Hm. For some reason these counts aren't showing up on the sandbox site.

+++ b/drupalorg_project/drupalorg_project.module
@@ -841,6 +861,14 @@ function drupalorg_project_bingo_block_output() {
+               'status' => array(1,2,8,13,14,15),

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.

webchick’s picture

Also, 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"?

xjm’s picture

So how about we move the novice link up closer to the top, right under "Your issues"?

I think that pattern would be more intuitive (and has the advantage of promoting Novice issues as well!).

Anonymous’s picture

Status: Needs work » Needs review
FileSize
5.74 KB

And 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!

dww’s picture

Status: Needs review » Needs work
+  $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"));

Since 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

Anonymous’s picture

Status: Needs work » Needs review
FileSize
5.76 KB

Thanks 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!

webchick’s picture

Awesome! I'm having fun, too! :)

The latest patch had a syntax error in it. Oops. :)

+  $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)));

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:

SELECT name FROM users WHERE uid = 1 AND mail = 'bob@example.com';

Would become:

db_query("SELECT name FROM users WHERE uid = %d AND mail = '%s'");

And then you'd add the arguments one after another at the end:

db_query("SELECT name FROM users WHERE uid = %d AND mail = '%s'", $user->uid, $user->mail);

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.

webchick’s picture

Ahem. :P How about a patch without all the other crap from my sandbox? ;)

webchick’s picture

Status: Needs review » Needs work

Hm. 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.

webchick’s picture

This 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. ;)

webchick’s picture

OH. 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

Anonymous’s picture

Status: Needs work » Needs review
FileSize
5.75 KB

Wow, 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! ;)

dww’s picture

Status: Needs review » Needs work

Nice. 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. ;)

+  $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' AND sid IN (1,2,8,13,14,15), "Novice"));

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:

+  $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 = 'Novice' AND sid IN (1,2,8,13,14,15)"));

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:

+  $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 = 'Novice' AND pi.sid IN (1,2,8,13,14,15)"));

(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:

+  $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 = 'Novice' AND pi.sid IN (1, 2, 8, 13, 14, 15)"));

Make sense?

Thanks,
-Derek

webchick’s picture

Oh, my advice was pwned by dww. Nice! :)

webchick’s picture

Sandbox 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

Anonymous’s picture

Thanks 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. ;)

Anonymous’s picture

Status: Needs work » Needs review
webchick’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +needs drupal.org deployment

Perfect!! 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.

Anonymous’s picture

Very 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.

drumm’s picture

Status: Reviewed & tested by the community » Fixed

Committed and deployed. I'll let the cache clearing run its course to get the counts updated.

webchick’s picture

YES!!! Thank you Neil!! GREAT job, demarcoz! Hope to see more patch contributions from you in the future. :D

tsvenson’s picture

I 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.

SeanBannister’s picture

I 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.

dww’s picture

Status: Fixed » Needs work
Issue tags: -needs drupal.org deployment

Whoops, 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

webchick’s picture

demarcoz thought that fixed issues were useful, so that new novices could see what other novices had done. I tend to agree.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
1.73 KB

As 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

dww’s picture

Status: Needs review » Fixed

Thanks. Reviewed, committed, and deployed.

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

x

  • Commit 5228ae9 on 6.x-3.x, 7.x-3.x-dev by drumm:
    #1266380 by demarcoz. Create a link to Novice issues in the contributor...
  • Commit f7830f7 on 6.x-3.x, 7.x-3.x-dev authored by demarcoz, committed by dww:
    [#1266380] by demarcoz: Follow-up to remove fixed issues from Novice...