One of the TODO items from #157693: Upgrade project issue to Drupal 6.x that could be easily done in parallel is documenting the schema in project_issue.install. This isn't critical before the d.o upgrade, but it'd be important before the 6.x-1.0 release.

Comments

mr.baileys’s picture

Status: Active » Needs review
StatusFileSize
new11.74 KB

Patch attached. The only field I was unsure about is {project_issue_state}.author_has.

aclight’s picture

Status: Needs review » Needs work

The only field I was unsure about is {project_issue_state}.author_has.

That means whether or not the author of an issue has the permission to set the state of an issue to a certain value. For example, you might have a system where users can create issues to report bugs but wouldn't normally have the permission to change the status (that would be the developers job). But you might want the original creator of an issue to be able to close it, if for example he realized that he reported a bug that wasn't really a bug in the first place.

Some other comments:

  1. Instead of + 'description' => t("Table containing issue-related settings per project."), I'd go with + 'description' => t("Table containing per-project issue-related settings."),
  2. +        'description' => t("A serialized array containing the types of issues to include in the email."),
    

    You're being inconsistent with "email" vs "e-mail" (see #363937: Use "e-mail" in all user facing strings). Also, at a quick glance it might not be obvious that the email mentioned here is the one sent to {project_projects}.mail_copy and not the emails sent to subscribers. So you might use this instead:

    +        'description' => t("A serialized array containing the types of issues to include in the e-mail sent to {project_projects}.mail_copy."),
    

    Both of these comments also apply to the next field,

    +        'description' => t("A serialized array containing the statuses to include in the email."),
    
  3. +        'description' => t("Component of the issue as defined on a project-by-project basis in {project_issue_projects}.components."),
    

    I'd instead go with

    +        'description' => t("Component of the issue as defined on a per-project basis in {project_issue_projects}.components."),
    

    Likewise, the same change would apply to

    +        'description' => t("Component of the issue after this comment was made. Components are defined on a project-by-project basis in {project_issue_projects}.components."),
    
  4. +        'description' => t("The {project_release_nodes}.rid (version identifier) for this issue (only used in conjunction with the project_release module). References project_release_nodes."),
    

    ... project_release nodes. No underscore between release and nodes.

  5. +        'description' => t("A serialized object containing the original values that were used when this issue was created."),
    

    I think "... original metadata values ..." might make this a bit more clear.

  6. For {project_issue_comments}, most of the descriptions have "after this comment was made" at the end of them. I wonder if we can come up with something a bit more clear here. I'm not sure if this is an improvement, but for "The title of the issue after this comment was made." we might instead have "The title of the issue set by a comment.". The somewhat tricky thing is that the user may not be changing the metadata in a comment, but from the back end the data is stored just as if the user *had* changed every metadata value. In other words, we're not storing the diff of the metadata, but the value of each metadata field set by each comment.
  7. +    'description' => t("Table keeping track of subscriptions per user per project for issue updates."),
    
  8. Maybe this is better

    +    'description' => t("Table keeping track of per-user project_issue subscriptions."),
    
  9. +        'description' => t("The type of subscription. Possible values are: 0 = not subscribed, 1 = subscribed to own issues or 2 = subscribed to all issues."),
    

    Maybe

    +        'description' => t("The type of subscription for the project. Possible values are: 0 = not subscribed, 1 = subscribed to own issues or 2 = subscribed to all issues."),
    

By the way, I believe the coding standards specify that whenever possible, single quotes should be used for strings instead of double quotes. My understanding is that there are no longer any real performance reasons to do this, but I could be mistaken. dww and hunmonk are usually pretty strict about following the coding standards, but on this issue I am not sure if they really care.

dww’s picture

Status: Needs work » Fixed

A) I added the blurb for "author_has":

t('Boolean indicating whether or not the author of an issue may use this issue status in that issue, regardless of the permissions controlling site-wide use of this status.')

(that's a little clumsy, further edits would be welcome). ;)

B) I converted t("...") to t('...') since that's the standard unless you need "" for some reason.

C) Made a few minor edits for clarity.

Committed to HEAD.

Thanks!
-Derek

mr.baileys’s picture

Component: Miscellaneous » Documentation
Priority: Normal » Minor
Status: Fixed » Needs review
StatusFileSize
new3.75 KB

Thanks for the detailed review & feedback, very much appreciated! Based on the feedback, I made some additional changes to HEAD (patch attached):

1,2 & 3) fixed inconsistent spelling of "e-mail" & improvements to wording.

4) Actually, the "References project_releases node"-part can be dropped altogether since the description starts with "The {project_release_nodes}.rid (version identifier) for this issue...". Removed it from the description

5) changed description to read "...original metadata values..."

6) I agree that "...after this comment was made." sounds a bit akward. Not sure about your suggestion

The title of the issue set by a comment.

though: although correct from a code-perspective, from a data-perspective the title could have remained unchanged. I'd like to have this description convey that the comment might have changed the title of the issue, and that this title in turn could have been changed again by further comments to this issue. Unfortunately, I can't seem to come up with a decent way to phrase this... I would also use "this comment" instead of "a comment".

7) Yes, that does sound better, changed it.

Regarding quotes and coding standards: my initial changes had some single-quotes in the descriptions, and since the coding standards mentioned the following exception to the "single-quotes preferred"-rule

Translated strings where one can avoid escaping single quotes by enclosing the string in double quotes. One such string would be "He's a good person." It would be 'He\'s a good person.' with single quotes. Such escaping may not be handled properly by .pot file generators for text translation.

I'd converted all descriptions to double-quotes. The single-quotes in the descriptions have since been removed though, so I'm ok with the single quotes wrapping the descriptions.

aclight’s picture

though: although correct from a code-perspective, from a data-perspective the title could have remained unchanged. I'd like to have this description convey that the comment might have changed the title of the issue, and that this title in turn could have been changed again by further comments to this issue. Unfortunately, I can't seem to come up with a decent way to phrase this... I would also use "this comment" instead of "a comment".

Well, the point I was trying to make was that whether or not the user actually changes the value of any of the metadata in a comment, it is all stored in the database just as if the user had changed the values. I don't really like my proposed phrasing that much better than yours, and if neither of us can come up with something better it's probably not worth spending more time on it, since this is an extremely minor point. I just wanted to bring it up previously in case you were able to come up with something snappy :)

dww’s picture

Status: Needs review » Fixed

@aclight #2: wow, sorry, I totally missed that you had commented here while I was reviewing and committing #1... sorry about that! All good points, glad they're now fixed. ;)

@mr.baileys: Yes, I'd normally just use " all the time for t() (to avoid the t('ain\'t this a shame') problem) if He-who-must-not-be-named and others didn't come screaming in my issue queue about it. :/

Thanks to both of you for your help here, committed #4 to HEAD.

Status: Fixed » Closed (fixed)

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