This is a follow-on to work in #1986334: Allow a particular revision of a fieldable panels pane to be placed in a panelized page

I think there are several possible UX problems that arise from those changes. I'll start by outlining them here. Depending on the discussion, it may make sense to split some out into their own issues.

  1. One use-case the work in 1986334 is meant to serve is to use FPP with Panelizer and a revisioning system (like Workbench Moderation) to allow users to edit non-reusable FPPs on draft revisions without having edits affect all the revisions on which that FPP might be embedded. Indeed, I anticipate that the user would expect an edit to an FPP to only affect the revision of the host entity on which they're currently working.

    Once the patch in 1986334 is committed this will theoretically be possible. That patch allows the admin to change a setting to "lock" the FPP embed to a specific revision ID when it's embedded. But to achieve the use-case as-described, the content editor must choose to create a new FPP revision any time they're editing an FPP, otherwise their edits will change the revision of the FPP, and be reflected on all other node revisions that have that same FPP revision embedded. I don't see any reason to expose the editor to this decision; we're just setting them up for failure. I submit that, if an admin has chosen to "lock" FPP revisions then FPP should automatically create a new revision every time the FPP is edited. In other words, we should prevent the editing of an non-reusable FPP revision -- once a revision is created it will never change, and any changes require we auto-create a new revision. This allows content editors to be confident that their edits won't have unexpected consequences by affecting other revisions of the host entity.

  2. The patch currently under consideration in 1986334 is #79 and it adds the ability to "lock" the revision of reusable panes. I'm not clear on the purpose of this, and I think it's going to lead to some significant confusion. The whole point of reusable panes is to be able to embed them once then have a single update to the pane affect all places where that pane is embedded. FPP reusable panes have their own sort of revision workflow in place that allow editors to make a given FPP revision the "current" revision, thus changing all places where that pane is embedded. With the new "locking" functionality the difference between reusable and non-reusable panes is muddied. There's no indication in the interface as to what FPP revision a given embed is "locked" to, and, moreover, changing the which FPP revision is "current" for a reusable pane will have no effect on the embeds where the FPP revision is locked. It's all very confusing; content editors are never going to be able to untangle this. My suggestion: remove this functionality. I see no good reason to be able to lock the revision of reusable FPPs when they are embedded. Alternatively: can someone provide a description of a use-case this is meant to serve, and a reasonable workflow a user is meant to take to achieve such a use-case?

  3. There's a problem of distinguishing between reusable and non-reusable FPPs in the interface. Once an FPP is created it's not apparent, when editing it, whether it's a reusable FPP or a non-reusable FPP. If my suggestion from #1 is taken, then editing a non-reusable FPP will always be "safe" -- such edits would only apply to the immediate embedding context. But editing a reusable FPP has potential side-effects since it could be embedded on any number of entities or revisions. The difference between these different types of FPPs should be made much clearer to the user. One thing I've done is prevent editing reusable FPPs in the Panels interface itself, and I link users to the reusable FPP listing page to do their edits there. I find the change of context helps them understand the difference in the types of FPPs. There may be other satisfactory solutions that are less drastic than that.

Summary of proposed steps to solve these problems

  1. If the setting to "lock" embeds of non-reusable FPPs to specific revisions is enabled, then remove the "create new revision" checkbox from the FPP edit form for these FPPs and just always create a new revision when editing these FPPs.
  2. Remove the ability to choose to "lock" embeds of reusable FPPs to specific revisions.
  3. This is more controversial, but prevent users from editing reusable FPPs in the Panels interface. Or at least provide very clear messaging that this is a reusable pane and that edits might be made immediately public, and affect other host entities.
CommentFileSizeAuthor
#46 fieldable_panels_panes-n2695499-46.patch18.41 KBDamienMcKenna
#45 fieldable_panels_panes-n2695499-45.interdiff.txt2.45 KBDamienMcKenna
#45 fieldable_panels_panes-n2695499-45.patch14.45 KBDamienMcKenna
#43 fieldable_panels_panes-n2695499-43.interdiff.txt1.83 KBDamienMcKenna
#43 fieldable_panels_panes-n2695499-43.patch16.66 KBDamienMcKenna
#42 fieldable_panels_panes-n2695499-41.interdiff.txt2.03 KBDamienMcKenna
#42 fieldable_panels_panes-n2695499-41.patch17.16 KBDamienMcKenna
#40 fpp-n2695499-example.png60.95 KBDamienMcKenna
#39 fieldable_panels_panes-n2695499-39.interdiff.txt2.48 KBDamienMcKenna
#39 fieldable_panels_panes-n2695499-39.patch16.94 KBDamienMcKenna
#38 fieldable_panels_panes-n2695499-38.interdiff.txt847 bytesDamienMcKenna
#38 fieldable_panels_panes-n2695499-38.patch15.69 KBDamienMcKenna
#36 fieldable_panels_panes-n2695499-36.interdiff.txt701 bytesDamienMcKenna
#36 fieldable_panels_panes-n2695499-36.patch15.69 KBDamienMcKenna
#35 fieldable_panels_panes-n2695499-35.interdiff.txt4.31 KBDamienMcKenna
#35 fieldable_panels_panes-n2695499-35.patch15.69 KBDamienMcKenna
#34 fieldable_panels_panes-n2695499-34.interdiff.txt1.2 KBDamienMcKenna
#34 fieldable_panels_panes-n2695499-34.patch15.34 KBDamienMcKenna
#33 fieldable_panels_panes-n2695499-33.patch15.17 KBDamienMcKenna
#33 fieldable_panels_panes-n2695499-33.interdiff.txt808 bytesDamienMcKenna
#32 fieldable_panels_panes-n2695499-32.interdiff.txt1.56 KBDamienMcKenna
#32 fieldable_panels_panes-n2695499-32.patch15.17 KBDamienMcKenna
#31 fieldable_panels_panes-n2695499-31.patch14.17 KBDamienMcKenna
#31 fieldable_panels_panes-n2695499-31.interdiff.txt3.07 KBDamienMcKenna
#30 fieldable_panels_panes-n2695499-28.actual-interdiff.txt3 KBDamienMcKenna
#29 fieldable_panels_panes-n2695499-28.interdiff.txt13.83 KBDamienMcKenna
#28 fieldable_panels_panes-n2695499-28.patch13.83 KBttamniwdoog
#26 fieldable_panels_panes-n2695499-26.patch12.92 KBttamniwdoog
#20 fieldable_panels_panes-n2695499-20.patch11.61 KBDamienMcKenna
#14 fieldable_panels_panes-n2695499-14.patch2.49 KBDamienMcKenna
#11 fieldable_panels_panes-n2695499-10.patch2.09 KBDamienMcKenna
#8 fieldable_panels_panes-n2695499-8.patch1.37 KBDamienMcKenna
#7 fieldable_panels_panes-n2695499-7.patch1.33 KBDamienMcKenna
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

azinck created an issue. See original summary.

azinck’s picture

Issue summary: View changes
azinck’s picture

Issue summary: View changes
azinck’s picture

Issue summary: View changes
azinck’s picture

Issue summary: View changes
azinck’s picture

Issue summary: View changes
DamienMcKenna’s picture

Status: Active » Needs review
FileSize
1.33 KB

Yes, thanks for starting this follow-on issue.

This adds a suffix to the title of all FPPs to indicate whether it's reusable or not reusable, and then adds a bit to the end of the 'info' text to indicate both the reusability and the revision state.

DamienMcKenna’s picture

A minor improvement to the message.

azinck’s picture

Status: Needs review » Needs work

Thanks for the patch Damien. Your patch claims that if an FPP is locked then it will be saved as a new revision. But you did not actually add in the logic to achieve that functionality...and I don't think that exists anywhere in any other patches to date, does it?

azinck’s picture

My other feedback: your messaging focuses on how "immediately" a change takes effect. It's not so much about "immediateness" as it is about the *breadth of the scope* to which the change applies. For non-locked panes the change will apply to *all* places the pane is embedded. For locked panes the affected scope will only be places where the particular revision is embedded.

And that's why I'm pretty strongly arguing that non-reusable panes should always be "locked" and get a new revision when edited, and why reusable panes should never have the option to be locked. Because otherwise non-reusable panes start crossing into reusable territory when dealing with revisions on the host entity, and reusable panes can start crossing into non-reusable territory.

DamienMcKenna’s picture

Yeah, I was in a meeting while doing the wording, so I missed the point.

Does this work better?

DamienMcKenna’s picture

Status: Needs work » Needs review
DamienMcKenna’s picture

Status: Needs review » Needs work

@azinck: I see your point.

DamienMcKenna’s picture

Status: Needs work » Needs review
FileSize
2.49 KB

This changes the note if the pane is reusable.

DamienMcKenna’s picture

@azinck: After studying your comments in more detail, I wholeheartedly agree with you. We'll be working towards this for the next release.

DamienMcKenna’s picture

Status: Needs review » Needs work

FYI we need indications on both the pane and the FPP edit to explain what's going.

DamienMcKenna’s picture

Issue tags: +Needs tests

We also made a mistake in the other issue by not adding tests first.

DamienMcKenna’s picture

One problem I see with the current functionality when using e.g. Workbench Moderation:

  • Enable 'lock' mode.
  • Update an existing Panelizer display for a node.
  • Edit a non-reusable FPP, save it as a new revision.
  • Save the Panelizer changes as a new revision.
  • The FPP changes are made visible.

This happens because the {panels_pane} records for non-reusable panes are not updated to point to a specific revision, they still point to the generic "fpid" value, so when a new revision is created the old panes will grab the new version.

I think we need an update script to update all existing {panels_pane} records for non-reusable panes and are tied to a {panelizer_entity} record to point to the correct vid instead of the fpid.

azinck’s picture

Yeah, I wondered about how much we want to support switching the locking mode of existing embedded FPPs. It would make things simpler if the new "locking" behavior weren't a setting at all that could be toggled at the user's whim. We could just decide that all non-reusable panes are now going to be referenced by vid, set up an update hook to fix existing embeds, then leave it at that. But such a change would probably demand a new major version.

DamienMcKenna’s picture

Status: Needs work » Needs review
FileSize
11.61 KB

This makes a few changes:

  • The default 'lock' option has been changed to lock all non-reusable FPPs instead of sticking with the legacy method.
  • An update script is provided to set existing sites to 'legacy' mode, to avoid conflict with the new default.
  • The option has been removed that would have allowed reusable FPPs to have their revision locked.
  • An option has been added to hide the contextual menus for FPPs as editing them directly could lead to workflow confusion.
  • An option has been added to stop the "reusable" state of panes to be changed once they're created.
  • Two stub update scripts are provided for updating Panelizer and Panel Nodes to use the vid of non-reusable panes instead of their fpid; this code will be forthcoming.
  • Some text has been added to the pane's display to indicate whether the pane is reusable and what the effect will be if a change is made.

Still need to manually test it and write actual tests for the changes.

azinck’s picture

Thanks for all your work, Damien. These are just the sort of improvements I was hoping for. I'll do some testing.

heddn’s picture

On those update hooks, I'd like to suggest we use the batch api and $sandbox. We didn't for panels_update_7302 and #2192993: Change panels_update_7302 to use a batch to update records is still opened to fix that.

DamienMcKenna’s picture

@heddn: Yes, they will when they're finished, I just included that code to provide a stub for someone else who's working on the actual update scripts.

DamienMcKenna’s picture

I'm wondering if we should get rid of the "current:[FPID]" logic and just stick with either fpid or vid?

DamienMcKenna’s picture

Was anyone able to test out the changes yet? I haven't gotten to give them a proper test run yet, and might not get to 'til next Tuesday-ish.

ttamniwdoog’s picture

Updated fieldable_panels_panes_update_7112 to:

  1. Search for all Panelizer displays that use a non-reusable FPP.
  2. Update the panels_pane record so it says "vid:VID" instead of "fpid:FPID".
  3. The VID should come from the fieldable_panels_pane record.
DamienMcKenna’s picture

Thanks ttamniwdoog. Some feedback:

  1. +++ b/fieldable_panels_panes.install
    @@ -428,3 +429,61 @@ function fieldable_panels_panes_update_7109() {
    +function fieldable_panels_panes_update_7112() {
    

    This needs to use a $sandbox.

  2. +++ b/fieldable_panels_panes.install
    @@ -428,3 +429,61 @@ function fieldable_panels_panes_update_7109() {
    +  $fpids = array();
    

    Don't need this variable.

  3. +++ b/fieldable_panels_panes.install
    @@ -428,3 +429,61 @@ function fieldable_panels_panes_update_7109() {
    +  $results = db_query('SELECT fpid, vid FROM {fieldable_panels_panes} WHERE reusable <> 1');
    

    The original plan was to only check panes used in Panelizer displays, but we need to see if non-reusable FPPs added to a normal Panels page also uses the vid or fpid.

ttamniwdoog’s picture

Thanks for all your help DamienMcKenna.
Here are the changes from your feedback.

DamienMcKenna’s picture

Here's an interdiff between #20 and #28.

DamienMcKenna’s picture

Doh, sorry, *this* is the interdiff.

DamienMcKenna’s picture

DamienMcKenna’s picture

This adds a comment to the update script explaining how to make it work. We'll need a change record for this too that include the code.

DamienMcKenna’s picture

DamienMcKenna’s picture

DamienMcKenna’s picture

This fixes problems with the workflow status indicators being added to the rendered pane (whoops!).

DamienMcKenna’s picture

This fixes the comment in the update script related to hook_update_dependencies(). Also, doh.

DamienMcKenna’s picture

This could use some usability review.

DamienMcKenna’s picture

This fixes a bug in the update script that changed the strings "fpid:" and "current:" to "vid:" but didn't update their IDs. Dangit. Sorry.

DamienMcKenna’s picture

This changes the comments in update_7112 to note a simpler approach to running the updates and notes the problems with hook_update_dependencies(). It also changes update_7111 to not override the variable if it has been set already.

DamienMcKenna’s picture

FileSize
60.95 KB

This is an example of what it looks like:

Example screenshot of what the UX changes look like.

First off it says either "(reusable)" or "(not reusable)" depending upon the FPP's status. It then also adds a suffix to the pane's info to explain what happens when the pane is edited and the page is saved.

azinck’s picture

Thanks for continuing to move this forward, Damien.

I think the language for the non-reusable pane is too complicated. What about just ditching all of that help text entirely. I think users generally expect their edits to only apply to the current context, so we need only call out the different behavior for the reusable panes.

If you don't like the idea of dropping the text for non-reusable panes, then what about: "Edits to this pane will only apply to this revision of this entity."

DamienMcKenna’s picture

This makes a few small adjustments:

  • It adds a description to the "Make this entity reusable" checkbox to better explain what the option means.
  • It removes the "(not reusable)" message on FPP pane titles; dsnopek suggested it could be displayed so often that it would lead to being ignored as UX noise.
DamienMcKenna’s picture

This removes the option to allow FPP entity reusablity to be changed again, i.e. forcing FPPs so that the "reusable" setting cannot be changed after it is initially set.

DamienMcKenna’s picture

I have two change records logged related to this and #1986334.

DamienMcKenna’s picture

This reverts the UX changes, except for the indicator on reusable panes, that's staying.

DamienMcKenna’s picture

This finishes fieldable_panels_panes_update_7113 to update Panel Nodes displays to use the new reference structure.

  • DamienMcKenna committed 62248ad on 7.x-1.x
    Issue #2695499 by DamienMcKenna, ttamniwdoog, azinck, heddn: On new...
DamienMcKenna’s picture

Status: Needs review » Fixed

Committed. We'll deal with further improvements in other issues.

Status: Fixed » Closed (fixed)

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

azinck’s picture

Just wanted to chime in that I've just now had the chance to look more closely at this code and test it and it looks great. I know it already got committed, but it gets an additional thumbs-up from me. Thanks Damien.