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.

CommentFileSizeAuthor
#20 pi_state_to_status-20.patch51.36 KBmikey_p
#15 pi_state_to_status-15.patch51.21 KBmikey_p
#13 pi_state_to_status.patch54.58 KBhunmonk
#6 issue-353248-table-rename.patch664 byteswillmoy
#3 issue-353248.patch48.33 KBwillmoy
#3 patterns_sed.txt1009 byteswillmoy

Comments

dww’s picture

dww
we/he/they
commented
Component: Issues » User interface
Category: task » bug

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.

aclight’s picture

aclight commented
Issue tags: +6.x-1.0 blocker

Adding 6.x-1.0 blocker tag.

willmoy’s picture

willmoy commented
Assigned: Unassigned » willmoy
StatusFileSize
new patterns_sed.txt1009 bytes
new issue-353248.patch48.33 KB

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.

willmoy’s picture

willmoy commented
Status: Active » Needs review
dww’s picture

dww
we/he/they
commented
Status: Needs review » Needs work

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

willmoy’s picture

willmoy commented
StatusFileSize
new issue-353248-table-rename.patch664 bytes

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:

/**
 * 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;
}
dww’s picture

dww
we/he/they
commented
Status: Needs work » Needs review

Great, thanks. ;) Back to "needs review"...

marcp’s picture

marcp commented

Anyone get a chance to test these patches yet?

cweagans’s picture

cweagans
He/Him
Primary language English
Location Boise, ID, USA
commented

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.

mikey_p’s picture

mikey_p commented

subscribing, plan to test this tonight/tomorrow

hunmonk’s picture

hunmonk commented
Assigned: willmoy » hunmonk

related patch for pift that will need to be deployed at the same time #774230: sync with project_issue state -> status changes

dww’s picture

dww
we/he/they
commented

@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

hunmonk’s picture

hunmonk commented
StatusFileSize
new pi_state_to_status.patch54.58 KB

updated patch attached. basically combines the two other working patches, and fixes a missing variable rename in the update function.

dww’s picture

dww
we/he/they
commented
Status: Needs review » Needs work

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

mikey_p’s picture

mikey_p commented
Status: Needs work » Needs review
StatusFileSize
new pi_state_to_status-15.patch51.21 KB

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.

mikey_p’s picture

mikey_p commented

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

mikey_p’s picture

mikey_p commented
Status: Needs review » Needs work
mikey_p’s picture

mikey_p commented

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?

hunmonk’s picture

hunmonk commented

sure. just make sure any references to mail_copy_filter_state are also updated in the code (which i'm guessing they are already).

mikey_p’s picture

mikey_p commented
Status: Needs work » Needs review
StatusFileSize
new pi_state_to_status-20.patch51.36 KB

I did some basic clicking around that this seems fine, but of course could use tests.

hunmonk’s picture

hunmonk commented
Assigned: hunmonk » Unassigned
mustanggb’s picture

mustanggb commented