Closed (fixed)
Project:
Project issue tracking
Version:
7.x-2.x-dev
Component:
User interface
Priority:
Normal
Category:
Task
Assigned:
Reporter:
Created:
10 May 2012 at 23:31 UTC
Updated:
4 Jan 2014 at 02:05 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #1
senpai commentedAssigning and tagging.
Comment #2
rgristroph commentedAnother simple settings removal.
Test by going to admin/project/project-issue-settings ; before applying the patch, there should be a "Site-wide help" multi-line text box at the bottom, afterwards it should be gone. I checked the code with grep to make sure the variable that the form set was not used anywhere.
Comment #3
iamcarrico commentedTested, reviewed, he speaks the truth.
Comment #4
senpai commentedTested locally, and this patch does indeed remove the form element cleanly as expected.
Setting this one back to 'needs work' because I'm a little concerned about the data that might have been present in $form['project_issue_site_help']['#description] when the form element was nuked. We'd silently be killing all the documentation|instructions that someone so carefully typed without so much as a red flag warning.
Proposal: Let's add an update.php hook to transfer the contents of this textarea into the core-provided help text for each issue node type.
Comment #5
rgristroph commentedGood point, I'll do another patch following Senpai's suggestion.
Comment #6
rgristroph commentedIn the case where there is old help text, and the issue node types already have "Explanation or submission guidelines" set, what should we do ?
I think appending the old to the new and alerting the user via a message from the update hook is sufficient, but we might also want to consider doing nothing and alerting the user, or alerting the user more strongly via an an email the site email address.
Comment #7
dwwIf we do anything at all, I say just append and print a warning during update.php. No sense adding the complication of sending an email.
However, if you start with a D6 site and try to upgrade to D7, you're in a world of hurt right now. The new default node type(s) provided by Project* will directly conflict with the old D6 names, there's nothing to migrate, and in fact, the new node types are only created on hook_install(), not via hook_update_N(). So, I'm not convinced this is worth messing with at all until #1549402: Provide a D6 to D7 data migration path for project_issue and comments is further along. In fact, we should probably open a separate issue specifically about the creation of the new node types and all the field instances in the D6 -> D7 upgrade case. Since we need that to work even before migrate.module can do us any good.
Personally, I'd be fine just calling this done, and maybe open another task about migrating this data if we want to track that so it doesn't get lost.
Thanks,
-Derek
Comment #8
senpai commentedAfter an IRC convo to hash out the possibilities of this scenario *ever* hapening, we decided to append any old text to end of the new text rather than summarily deleting it, and set_message + watchdog the user from within the update hook.
BTW, what happens to the old text in the system settings form if the form element gets nuked? Does the old text just sit around in the db never to be seen or heard from again?
Comment #9
rgristroph commentedThis is the patch with the update hook added.
I tested it with having more than one project_issue node type, and having one of them have a help set and the other one didn't. I first set a project-wide issue help message in the project settings, then applied this patch, and then ran updatedb; I had to drush cc all twice after that.
The variable holding the old help message disappears (you can confirm that with drush vget before and after doing the drush updatedb), and the new help messages appear on the appropriate node/add/ forms. Also, checking Reports -> Recent log messages shows an appropriate watchdog message was generated.
Comment #10
rgristroph commentedI need to re-roll that patch to use l() instead of hand building the link - doing that now.
Comment #11
rgristroph commentedRe-rolled from the self applied code review :)
Now uses l(), should be up to snuff.
--Rob
Comment #12
iamcarrico commentedWell, this code is cool--- but isnt it unnecessary at it's complexity? If this is PURELY for D6->D7 upgrade, then project_issue_node_types() will either return nothing (as we changed the variable we use for this one...) or just project_issue as the node type. I think it would be far simpler to just take the $help_text and put it into the project_issue node type and call it done. Anything else is just planning for functionality that does not exist for D6...
Really, I think just a warning saying that this text will be lost and setting the new variable will be enough.
Comment #13
rgristroph commentedYeah, Senpai and I discussed those concerns and our probably over-kill in IRC, but we felt this was best. It is just in an update hook so it shouldn't have any performance implecations or anything like that.
--Rob
Comment #14
iamcarrico commentedYou will have to add in the code to set the project_issue node type to be a project issue within that update hook. Without it project_issue_issue_node_types() should return nothing.
Fix the one line that has too many spaces while you are at it, and with those two changes it should be good to go.
Comment #15
rgristroph commentedHere is the patch with some blank lines and extra spaces removed.
I discussed this with @ChingizKhan in person, and we agreed the setting of the variable he mentions in #12 and #14 would only be needed on the currently non-existent D6 -> D7 upgrade path, and that should be addressed separately whenever we do it. This has been tested upgrading within D7, with multiple project_issue types, some with site-wide help set and some without, and it handled all the cases.
--Rob
Comment #16
dwwThis was at 'needs work' but given the new patch, I figured I should review it.
I stand by my earlier comments that this update hook is either premature or entirely misguided. ;) I'm not sure why Senpai is trying to make more work for us here. As ChinggizKhan correctly points out, the first update hook that runs (7000) needs to see if the D6 hard-coded project_issue content type exists, and if so, migrate into the D7 default content type, set the appropriate settings, deal with the field instance configuration, etc, etc. Without that, this update hook doesn't do anything.
That said, if this became update 7001 or something:
A) There are a bunch of code style bugs (missing spaces after commas, funky multi-line indentation, extra space before ;, non-standard DBTNG query formatting, etc).
B) The drupal_set_message() should use t(), since watchdog() is already doing that for you. However, the way you're feeding it the string isn't ideal, since the "you should check that here:" stuff assumes left-to-right (LTR) languages. If you put the URL into the t()/watchdog() string as a placeholder, folks can properly translate this for right-to-left (RTL) languages, too.
C) Really:
"<br>\n"? Can't we just add some newlines? Doesn't the help text already get formatted such that newlines are converted into<p>or<br />automatically as appropriate? If we actually have to hard-code markup in there, it should at least be a self-closing<br />.---
So, I can't decide if the status should be "needs work" or just "postponed" pending an actual solution to #1549402: Provide a D6 to D7 data migration path for project_issue and comments (or at least a sub-task for migrating the content type configuration itself, much less the data). Also, if this never happens, we can still launch d.o on D7, so I'm down-grading the priority.
Thanks,
-Derek
Comment #17
dwwBTW, #15 no longer applied cleanly. Here's the re-roll I was reviewing.
Comment #18
rgristroph commentedHere's a patch addressing some of the coding style issues in the update hook - however I'm not marking the issue as needs review because I'm going to remove that update hook from this patch entirely and just do the settings change here. From https://drupal.org/node/1549402 I'll reference this patch should the update hook code be of any use when we do the migration path.
Comment #19
rgristroph commentedHere's a patch that simply removes the site wide help from the settings form. (It also adds a file comment block for project_issue.install because coder review complained about that.)
Comment #20
senpai commentedTagging for Sprint 6.
Comment #21
bdragon commentedCommitted, with slight comment whitespace change to match comment formatting standards.
http://drupalcode.org/project/project_issue.git/commit/e1f9cbb
It will be up to the migration code to use and then delete the variable.