Posted by dww on January 1, 2009 at 8:31pm
7 followers
| Project: | Project issue tracking |
| Version: | 6.x-1.x-dev |
| Component: | User interface |
| Category: | bug report |
| Priority: | normal |
| Assigned: | hunmonk |
| Status: | needs review |
| Issue tags: | 6.x-1.0 blocker |
Issue Summary
While looking through the code working on something else, I noticed that in some places, we call "active", "fixed", etc the "Status" of an issue. I believe we're fairly consistent about that in the UI, otherwise, I'd call this a bug. However, the underlying code sometimes calls it a "status" and other times calls it a "state". We should standardize completely, rename functions + variables, etc. We should do this during/after the D6 port.
Comments
#1
Bah, this is a UI bug, since there are a few places we use "state" in the UI:
A) At node/N/edit/issues, we use "States" as the header above the checkboxes, and mention "states" in the help text.
B) The help text for the "Auto-close days" setting at admin/project/project-issue-settings
C) The URL for issue query pages uses "states=1,2,3..." as the GET parameter name
I still think we should only fix this in D6, since it's going to cause URL link rot in the case of (C), which we're going to face anyway as we convert all issue listings to views.
#2
Adding 6.x-1.0 blocker tag.
#3
There are—
324 uses of 'state' in project_issue
178 left after you replace the strings as in patterns.sed
32 left after you ignore $form_state
Those 32 are mainly natural language (comments, t() strings) and I can patch those manually.
The rest can be solved by running:
find project_issue_HEAD/ -type f -exec sed -i -f patterns.sed {} \;
patterns.sed is attached as patterns_sed.txt. A patch against HEAD is also attached, if this is a convenient time to apply it.
I don't have an install I can test the patch on but I have proof read it carefully.
This patch is by way of meagre thanks for all your hard work on this system.
#4
#5
Very cool, thanks! One glaring omission from this patch is that if the following happens:
- $result = db_query('SELECT * FROM {project_issue_state} ORDER BY weight');- while ($state = db_fetch_object($result)) {
- $options[] = $state;
+ $result = db_query('SELECT * FROM {project_issue_status} ORDER BY weight');
+ while ($status = db_fetch_object($result)) {
+ $options[] = $status;
then we definitely need to add a project_issue_update_6003() to rename this table. ;)
Otherwise, quick skim looks pretty good... Don't have time right now for a more in depth review, but this is off to a great start, especially since it's automated as a sed script, not a big patch to manually reroll all the time. ;)
Thanks again!
-Derek
#6
Right you are. You see, if you want something done properly, you have to do it yourself ;)
Patch attached, for the table rename on its own. New function:
<?php/**
* Rename {project_issue_state} to {project_issue_status}
*/
function project_issue_update_6003() {
$ret = array();
db_rename_table($ret, 'project_issue_state', 'project_issue_status');
return $ret;
}
?>
#7
Great, thanks. ;) Back to "needs review"...
#8
Anyone get a chance to test these patches yet?
#9
The code looks solid to me. Not a whole lot that can really go wrong with this.
I'll try to test the patches tomorrow against HEAD. I doubt they will apply due to lots of recent activity in CVS, but we'll see.
#10
subscribing, plan to test this tonight/tomorrow
#11
related patch for pift that will need to be deployed at the same time #774230: sync with project_issue state -> status changes
#12
@mikey_p: Instead of just testing this manually, I have a proposal. ;) I think you should try to organize a team of folks willing to write tests for drupal.org at the code sprint on 4/18. You would delegate specific tests in project* to the team initially based on your efforts checking out this patch (and, time-permitting, write many of your own). Not only do we safely resolve a big 6.x-1.0 cleanup patch, we'd have a great start on a test suite for Project* that people can build on and flesh out throughout the rest of the day and at the QA sprints throughout the week. That'd be amazing on so many levels -- 6.x-1.0 progress, d.o redesign, d.o core upgrades, etc. Thanks in advance for trying to pull this off! ;)
Cheers,
-Derek
#13
updated patch attached. basically combines the two other working patches, and fixes a missing variable rename in the update function.
#14
As expected, this no longer applies now that #175555: Add custom Priority Levels landed. mikey_p was going to do the (mostly trivial) re-roll...
#15
Sorry I didn't get to this sooner, please feel free to bug me. I rolled this with git, and I'm not sure why it's smaller, I tried to use interdiff to see what the changes were, but it wasn't making much sense to me, and I didn't see anything that stood out. Of course at this point I'd strongly suggest that this has tests before being committed.
#16
I think I may have rolled back #780338: Need format_plural() when deleting an issue status in this patch, and it needs to be updated to include that. (I think that went in after the patch in #13).
#17
#18
After updating I was getting:
Notice: Undefined property: stdClass::$mail_copy_filter_status in project_issue_project_load() (line 35 of sites/all/modules/project_issue/includes/project_node.inc).This is because of {project_issue_projects}.mail_copy_filter_state not being update to status.
Is it okay to add the column change to the update function?
#19
sure. just make sure any references to mail_copy_filter_state are also updated in the code (which i'm guessing they are already).
#20
I did some basic clicking around that this seems fine, but of course could use tests.