Add a "draft" status for change records

Seems like a good idea. +1 to the proposal. Even without this being a policy change in core, this would be useful. ;)

However, the patch needs work:

A) (most importantly) This hunk reverts and re-introduces #2135759: DATA LOSS: Change record issue reference field is single-value:

@@ -1003,7 +1072,7 @@ function drupalorg_change_notice_field_default_fields() {
   $fields['node-changenotice-field_issues'] = array(
     'field_config' => array(
       'active' => '1',
-      'cardinality' => '-1',
+      'cardinality' => '1',
       'deleted' => '0',
       'entity_types' => array(),
       'field_name' => 'field_issues',

I know you're building this out on a dev site, but you have to make sure you're working against the very latest copy of the change records feature. You can checkout a copy of drupalorg directly from git into sites/default/modules/drupalorg, disable/re-enable this feature, and then revert it via the Features UI to (hopefully) get the dev site up to date with the latest code. Then re-create your feature. Or something...

B) Why have a whole new display for these? Why not just expose the filter on draft vs. committed/final (or whatever the term we're going to use is) on the existing view? Let the filter default to not-draft, but if people need to see draft ones, they can? Just seems likely to cause functionality drift between the two displays over time, and that it'll be easier to maintain and use if the change records are always in the same place, but you can specifically filter for the draft records if you need to.

Thanks,
-Derek

Reviewed this.

1. (separate issue) The "Add new change record" link appears even for anon users. Same as on d.o.
2. (separate issue) The Project field used to default to Drupal core and no longer does. Same as on d.o.
3. (deployment blocker) Unlike on d.o right now, using the Project autocomplete field here does not list "Drupal core" as the first result when entering "drupal" in the box. Instead it lists lowercase "drupal" which is node 2066111, not node 3060. Additionally, "Drupal core" is not anywhere in the list of autocomplete results since it cuts off after 10 results and seems to be sorting alphabetically. This is going to be a complete cluster-bleep if not fixed before deployment. :)
4. (separate issue) OMFFFFG @ not being able to enter a node ID for related issues. :( Same as on d.o.
5. (deployment blocker) We lost the ability to add multiple issues here.
6. (question) Why can you upload files here? I guess for images? Same as on d.o.
7. (regression) The "Impacts" field order got mixed up somehow. On d.o, it's above Updates done, but in the sandbox, it's at the very bottom, where it's likely to be missed.
8. (deployment blocker) Ok, I now made http://project-issues-drupal.redesign.devdrupal.org/node/2143393 as a draft. But there's no way to navigate to it that I can see. We should do one of the following, since manual intervention is required to uncheck that Draft checkbox, which may or may not ever actually get done:

   a) Add a tab to http://project-issues-drupal.redesign.devdrupal.org/list-changes/drupal to review draft notices.
   b) Include drafts in the main results listing but style them differently/prepend "[DRAFT]" to the title, etc.

   Basically, someone is going to need to sift through these prior to beta/RC and clean them out, so there needs to be a way to get at all of them quickly, as well as a way to clearly differentiate "this is an actual thing" vs. "this is still in progress."

   Nevermind, there's a filter there that I missed.

Also musing on whether it makes sense for "draft" to be the default. I guess that it does in the case of core once the policy change goes into effect, but not sure about contrib. Hm.

Yeah, sorry. I see now that the view has a filter for "Change record status" which I totally missed since it's at the very end. I haven't played much with D7 views in awhile; it it possible for this to be exposed as just a check box that says "Show drafts"? If not, it's probably fine; you can at least get to it, which was my concern. :)

Although, I guess just for completeness... the thing that made me log that as a deployment blocker is I created a change notice, then went to the list and didn't see it, so I worried my data was lost. I predict I won't be the first person to think something bad happened and file a bug report about it. Trying to think of what we could do to prevent this. Maybe a dsm() on create "Draft change notice created. You can view other drafts"? Not sure.

Btw, defaulting Draft to false would help alleviate those "lost data" concerns.

Re #9.4: See #2131965: add an Entity Reference Selection Handler for related issues. Now live. If you update your site to the latest project_issue code in the 7.x-2.x branch in Git (and drush cc all or enable/disable a module), you'll see a new EntityReference selection mode for your fields, and you can then do much nicer things with your issue references. It supports nid, #nid and title. It was supposed to support https://drupal.org/node/X too, and that worked on a dev site, but apparently it's barfing on production (for reasons my quick investigation couldn't explain).

Sounds like there's still a lot to be fixed before this is ready for actual review, but I'll leave the status alone for now...

FWIW, I like defaulting to draft, not live, for all the reasons xjm said in 18. I like the dsm() approach. It was my (stupid?) idea to have draft vs. live a filter on a shared view, but maybe it'd be clearer with two displays and menu tabs or something? But either way, the default display should be the live change records, whereas after you create something, it should start life draft (until vetted, etc). So, that's why I'm +1 to dsm(), and perhaps other solutions.

In fact, can we call this field something like field_publication_status and let it have actual values 'draft' and 'live' instead of double-negativing everything with a field called 'draft' and trying to make sense of TRUE/FALSE? ;)

Cheers,
-Derek

Couldn't we simply add "[DRAFT]" at the start of the title of change records that are not complete yet and be done with it? I'm sure it would be more than enough to signify such entries in the change record list and there would be no angry core developers eating anybody :P

I mean, sure yeah, storing the draft/live status as metadata somewhere is (or rather might prove to be) useful, and sure the whole save as draft by default unless specified otherwise does sound reasonable too, but the toggling of the view and the "View all draft change records" notice thing seem a bit too much trouble to me. Even the last screenshot above proves my point with the "[REVERTED]" keyword added to the title. It is more than enough to communicate what it means to.

So, how about we used the draft status to automatically prepend "[DRAFT]" to draft change records and remove it when they are set to live.

I agree with needing a status that does not go out in the feed and get tweeted.

I created a draft change notice once, and ... it did freak people out.

appending [DRAFT] to the title might be nice... but would have to be manually removed after the change notice is reviewed/approved. And, I bet we will see a lot of: draft -> live change, followed by edits to the title.

So maybe instead, we can put [DRAFT] in the rows when viewing change notices, and also somewhere *near* the title on the view of the change notice itself.

Just trying to play devil's advocate here... if a change notice got tweeted out with "[DRAFT]" in the title, why would anyone be angry/freak out? Wouldn't that actually be helpful, to call larger attention to issues that are at or near RTBC, to see if folks outside the 4-8 people participating have concerns about their implementation?

I actually think #22 is a simple, no-software-changes-required way to implement this policy, vs. holding it up on this implementation being deployed.

Because, as I mentioned before, it is *super* jarring to not see the content you just created, and I worry that between that and people forgetting to uncheck the checkbox, this method may result in confusion. Would be good to have a few more testers.

Hm. And regardless of which way we go on this, we'll need to watch the publish date of these. The listing/feed is sorted by create date, so if a draft is created way back when the issue first starts out (because the people involved are trying to be proactive) it might be days/weeks/months later by the time it actually ends up "live" in the main listing. If we go the software way of handling this (vs. the "title default text" way), maybe blank out $node->created if draft gets unchecked?

appending [DRAFT] to the title might be nice... but would have to be manually removed after the change notice is reviewed/approved....

I wasn't talking about prepending the string to the actual title and storing it in the db. I was talking about rendering it only during display (rewriting the title field in the view for example) for the change notices that had status = draft.

Wouldn't that actually be helpful, to call larger attention to issues that are at or near RTBC...

Precisely what I had in mind when I proposed adding "[DRAFT]" as opposed to hiding the change notifications till the very last moment.

Redirecting to the draft list after creating a draft could work, yep. Then at least you've seen it once, so if you don't see it on the main listing when you go back you go "Huh? OH! Duh. Filter." vs. "OMG BUGZ! To the d7qa queue!" ;)

And I agree there's a balance between perfection and getting something out there, and leave that up to you to manage. I'm just giving feedback.

screenshots look very reasonable.

What does the edit form look like? Does it have instructions on when to use draft vs published?

https://drupal.org/contributor-tasks/draft-change-notification mentions drafting a notice and posting back to the issue. Maybe we can link to that from the create/edit change notice form?

When changing from draft to published does it say what is about to happen (that a tweet will go out?) I was really surprised the first time that happened to me.

[edit]
Also, what does the *issue* look like? (when a draft change notice is made)

read through the code. (not with a fine tooth comb)

1. +++ b/features/drupalorg_change_notice/drupalorg_change_notice.features.field.inc
   @@ -2160,6 +2229,8 @@ function drupalorg_change_notice_field_default_fields() {
   +  t('By default, change records are created in a draft state. Making the change live will make it appear on the default change list.');
   +  t('Change record status');
   

   making the change "live"?

   we could say, changing the status from draft to published
   ...
   will make it...
   and a tweet goes out from @drupalWhatever

2. +++ b/features/drupalorg_change_notice/drupalorg_change_notice.install
   @@ -0,0 +1,27 @@
   +  // Re-save each as a live change record.
   +  $nodes = node_load_multiple($nids);
   +  foreach ($nodes as $node) {
   +    $node->field_change_record_status[LANGUAGE_NONE][0]['value'] = 1;
   +    node_save($node);
   +  }
   

   will this retrigger tweets?
   will it change their published dates?
   [edit to clarify] asking about update() that will resave all existing nodes

https://drupal.org/node/1643406 is one that should be changed to a draft once this is live :)

Reviewed this with Jesse Beach and Alex Bronstein today, with Jesse playing the role of "new user." Everything looks great, apart from one small suggestion. Since there's no one tasked with flipping the published bit process-wise, there's a good chance this is going to fall through the cracks from time to time. So it'd be good if on the referenced issue themselves, it was clear whether the referenced change notice was a draft or not.

A couple of ways this could be done:

1) A separate block that's "Draft change notices" vs. "Change notices" as the title.
2) Prepend "[Draft]" or something to the title of the issue in the block.

This way, a standing item during core mentoring hours could be to revisit fixed issues and make sure that their change notices were properly switched, and it'd be easy to tell.

When doing something that would require code changes anyway, why not add a view that lists fixed/closed issues that reference draft change notices?. Would be way more efficient to detect this I think?

There might be some false positives, with draft change notices also referencing related issues that were already fixed, but I don't think there are too many of those.

xjm also suggested that. my concern was exposing that in the UI in a sane way. No idea what the title of that tab would be.

Marking "needs work" to reflect that there is more to do here.

Just a minor suggestion: having a drop-down for only two choices requires two clicks to change. Seems to me that it would make more sense if we had some sort of toggle in place instead. If you agree, perhaps we could change it to a radio button set instead with the label placed above and the two options (published/draft) inline right below it.

...as I said: just a thought.

Thanx for taking the time to reply/explain.

Looking at the screenshot in #49.4, that feels strange to me. The reason is that "Add blah-dee-blah" is an "action link" pattern in D7+ and typically we do not put "View blah-dee-blah" stuff there. Those are instead generally done as tabs (see for example admin/comment vs. admin/comment/approval).

So borrowing from that standard UX practice in Drupal, and also to klonos's point about a drop-down w/ two items in it being weird, I would personally do this as 3 separate displays:

Published / Draft / Review

Because in the third instance, one of two things happened:

a) We just marked an issue as fixed, and its draft change notice is about to be published
b) Oops, we marked the issue fixed long ago and totally forgot to change its change notice status.

And in either case, it needs review. :)

I know dww previously said in #3.B that filters were better, but IMO that's mainly true if we're talking about 2 choices, and now we've upped it to 3. While the ongoing maintenance efforts are worth bearing in mind (like if we add a new column, we need to do it in 3 places now), I think this way would be more aligned with what core does, and also doesn't create any "secret handshake" knowledge that people need to know in order to help this process.

Committed & deployed.

drumm asked me to check out the newly deployed code. This one has two problems:

a) If I go to
https://drupal.org/list-changes
all is well.

But there is a link at the top there for "Review", which links to
https://drupal.org/list-changes/review

On that page it says:
Invalid project or no changes found.

That is misleading. I don't really know what it means actually. If there is no project, it should just default to Drupal Core like the other tabs do (or at least they appear to be doing), and if there are no changes, it should just tell me that.

b) The other problem is that if I start from a different project, like
https://drupal.org/list-changes/views
the links near the top for Draft and Review do not have the project in the URL so they are not going to the Draft or Review status changes for Views.

I recommend doing followups on a fresh dev site, after a snapshot is taken overnight. (Or after a couple drafts are posted, then overnight.) I can re-create the dev site tomorrow.

I'm also not seeing any "Review" change notices at all. It says they are for issues marked "fixed" and "closed/fixed" in the issue comments above, so there should be a ton of them for Drupal Core. Where are they?

And I created a test one in "draft" mode for the Simple Google Maps project. After creating it it gave me a link to "see all draft change notices" but it was empty, probably because it wasn't a Drupal Core change notice? I changed the project to Drupal Core and then the link was correct.

Review only lists *draft* (as the description says) notices that reference fixed issues, the idea is that they are now ready to be published. There are no draft change notices, so the list is obviously empty :)

Maybe needs a better title, but it is working as expected.

Well, that's confusing.

When I first went there, I didn't notice that the "review" records needed to be both draft and either fixed or closed/fixed. Maybe because the status value was in bold but the "draft" wasn't? That might have alleviated my confusion, and would be an easy fix to make.

And yes, the empty text also needs to be improved -- take out the "invalid project" part.

However, there are issues about the project (see #61 & 63) that are serious bugs that need to be addressed. It doesn't look like the Draft/Review tabs/links take into account the project at all.

#68 sounds good.

#69 doesn't sound good at all. What would you get if you clicked on Draft or Review and then clicked back on the Published link -- all notices or just Drupal Core? Because, unless I am mistaken, the Published page currently defaults to Drupal Core if no project is specified.

I think that is also happening for the other two tabs, actually. See #63 -- after creating a draft notice for a different project, it didn't show up.

Anyway... I think this change should be reverted for now. It is not really working in a sensible way. If tabs can't be used then figure out a different way to provide the links, but having Published be different from Draft/Review doesn't make any sense to me.

changerecords-drupal.redesign.devdrupal.org will be ready within 2 hours.

One thought on the menu tabs would be to use a Views Header that outputs markup very similar to Menu Tabs, but has the right links in it. Wouldn't that be fairly easy to do? I think you can use the views args, and/or fields from the first row, as placeholders in header text?

I don't like reverting things. It is almost always better to keep improving. And this added a field with data we don't want to throw away lightly.

(In hindsight, for future projects like this, I recommend try splitting into two phases. First, add only the field and let people start using the new data. Wait for some use in production, so the work on the Views will be informed by real data.)

See #63 - has a reproduce, 2nd paragraph. Sorry, can't work more on this today, urgent work came up.

what are the remaining tasks here?

Independently came across the bug described here at #2452133: "Drafts" tab on change records universally points to Drupal core's drafts, not a given project's. Though it's kind of a dupe it has a narrower scope and is therefore hopefully more actionable.

(I would add it as a related issue but that doesn't seem to work on the phone since the issue summary / relationships field set isn't expanding. The org credit worked though, nifty. :))

Yeah, well it's been a whole YEAR and the bugs have been ignored. Good luck.

Currently link to drafts is wrong for any draft CR in core - it points to "drafts" project CRs.

Reported in #2870439-73: Convert web tests to browser tests for config module

Confirm that, see https://www.drupal.org/node/2915307 where link leads to https://www.drupal.org/list-changes/drafts instead of https://www.drupal.org/list-changes/drupal/drafts

Found this issue from reading #2173425: [META] Merge DrupalMentoring.org features into Drupal.org.

Looks like everything has been done here. The points in the IS are complete.

The last few comments are about the link to drafts pointing to Core issues and not project issues. I double checked that by visiting the change notices for Drupal Commerce. The menu links are all project specific and I never wound up looking at only Core change records.

I think it is safe to close this as fixed.

It still needs work for breadcrumbs or tabs

steps

- visit https://www.drupal.org/list-changes/drupal/drafts - breadcrumbs is "Published"
- click first draft in table - atm https://www.drupal.org/node/3204546
- link in breadcrumbs leads to https://www.drupal.org/list-changes/drupal (published) but displays as "Change records" (expects "Draft")

- visit https://www.drupal.org/list-changes/drupal/review - breadcrumbs is "Published"
- click first CR to review - atm https://www.drupal.org/node/3195578
- link in breadcrumbs leads to https://www.drupal.org/list-changes/drupal (published) but displays as "Change records" (expects "Review")

@andypost, thanks for pointing that out. Since the problem with breadcrumbs is not mentioned in the IS or in any other comment in this issue shouldn't this be closed and a new issue opened for that?

I find it minor, so follow-up sounds better

Thanks, created a follow up and closing this as fixed. #3205291: Fix breadcrumbs on change notices