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.
-
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.
-
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?
-
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
- 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.
- Remove the ability to choose to "lock" embeds of reusable FPPs to specific revisions.
- 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.
Comment | File | Size | Author |
---|---|---|---|
#46 | fieldable_panels_panes-n2695499-46.patch | 18.41 KB | DamienMcKenna |
|
Comments
Comment #2
azinck CreditAttribution: azinck commentedComment #3
azinck CreditAttribution: azinck commentedComment #4
azinck CreditAttribution: azinck commentedComment #5
azinck CreditAttribution: azinck commentedComment #6
azinck CreditAttribution: azinck commentedComment #7
DamienMcKennaYes, 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.
Comment #8
DamienMcKennaA minor improvement to the message.
Comment #9
azinck CreditAttribution: azinck commentedThanks 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?
Comment #10
azinck CreditAttribution: azinck commentedMy 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.
Comment #11
DamienMcKennaYeah, I was in a meeting while doing the wording, so I missed the point.
Does this work better?
Comment #12
DamienMcKennaComment #13
DamienMcKenna@azinck: I see your point.
Comment #14
DamienMcKennaThis changes the note if the pane is reusable.
Comment #15
DamienMcKenna@azinck: After studying your comments in more detail, I wholeheartedly agree with you. We'll be working towards this for the next release.
Comment #16
DamienMcKennaFYI we need indications on both the pane and the FPP edit to explain what's going.
Comment #17
DamienMcKennaWe also made a mistake in the other issue by not adding tests first.
Comment #18
DamienMcKennaOne problem I see with the current functionality when using e.g. Workbench Moderation:
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.
Comment #19
azinck CreditAttribution: azinck commentedYeah, 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.
Comment #20
DamienMcKennaThis makes a few changes:
Still need to manually test it and write actual tests for the changes.
Comment #21
azinck CreditAttribution: azinck commentedThanks for all your work, Damien. These are just the sort of improvements I was hoping for. I'll do some testing.
Comment #22
heddnOn 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.
Comment #23
DamienMcKenna@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.
Comment #24
DamienMcKennaI'm wondering if we should get rid of the "current:[FPID]" logic and just stick with either fpid or vid?
Comment #25
DamienMcKennaWas 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.
Comment #26
ttamniwdoog CreditAttribution: ttamniwdoog at Mediacurrent commentedUpdated fieldable_panels_panes_update_7112 to:
Comment #27
DamienMcKennaThanks ttamniwdoog. Some feedback:
This needs to use a $sandbox.
Don't need this variable.
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.
Comment #28
ttamniwdoog CreditAttribution: ttamniwdoog at Mediacurrent commentedThanks for all your help DamienMcKenna.
Here are the changes from your feedback.
Comment #29
DamienMcKennaHere's an interdiff between #20 and #28.
Comment #30
DamienMcKennaDoh, sorry, *this* is the interdiff.
Comment #31
DamienMcKennaSome improvements to the update script.
Comment #32
DamienMcKennaThis 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.
Comment #33
DamienMcKennaA minor update to the status message at the end.
Comment #34
DamienMcKennaThis will also update the 'current:' records.
Comment #35
DamienMcKennaThis fixes problems with the workflow status indicators being added to the rendered pane (whoops!).
Comment #36
DamienMcKennaThis fixes the comment in the update script related to hook_update_dependencies(). Also, doh.
Comment #37
DamienMcKennaThis could use some usability review.
Comment #38
DamienMcKennaThis fixes a bug in the update script that changed the strings "fpid:" and "current:" to "vid:" but didn't update their IDs. Dangit. Sorry.
Comment #39
DamienMcKennaThis 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.
Comment #40
DamienMcKennaThis is an example of what it looks 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.
Comment #41
azinck CreditAttribution: azinck commentedThanks 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."
Comment #42
DamienMcKennaThis makes a few small adjustments:
Comment #43
DamienMcKennaThis 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.
Comment #44
DamienMcKennaI have two change records logged related to this and #1986334.
Comment #45
DamienMcKennaThis reverts the UX changes, except for the indicator on reusable panes, that's staying.
Comment #46
DamienMcKennaThis finishes fieldable_panels_panes_update_7113 to update Panel Nodes displays to use the new reference structure.
Comment #48
DamienMcKennaCommitted. We'll deal with further improvements in other issues.
Comment #50
azinck CreditAttribution: azinck commentedJust 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.