We are using Fieldable Panels Panes, Panelizer and Workbench Moderation in tandem to build a site with a moderation workflow in which a draft of the entire page is created and then reviewed before being published.
It works well, except when a fieldable panels pane is edited on the draft version of the page, the edits automatically appear on the published version of the page too. This is because the panelized page references the entity ID of the fieldable panels pane and therefore always uses the most recent revision.
I propose allowing the panelized page to refer to the revision ID of the fieldable panels pane instead. That way the draft revision of the node and the published version of the node will refer to different revisions of the FPP when the FPP is edited, and everything magically works.
Comment | File | Size | Author |
---|---|---|---|
#86 | interdiff-79-86.txt | 1.58 KB | slucero |
#86 | fieldable_panels_panes-n1986334-86.patch | 8.9 KB | slucero |
|
Comments
Comment #1
David_Rothstein CreditAttribution: David_Rothstein commentedSo, the Fieldable Panels Panes module almost supports this already. Here's a patch demonstrating a small tweak that makes it actually support it.
This patch is fine for our use case, but would need to be developed further in order to be committed to the module. Instead of forcing the revision ID to be stored in all cases like I'm (hack-ily) doing in the attached patch, it would presumably need to be some kind of option.
Comment #2
David_Rothstein CreditAttribution: David_Rothstein commentedAnother issue (credit goes to grndlvl for thinking of this) is that this model kind of breaks down for reusable FPPs... There, the goal (even for our use case) actually sort of is that if you edit the FPP in one place, your edits take effect everywhere at once.
Comment #3
merlinofchaos CreditAttribution: merlinofchaos commentedYou have just discovered the use-case for ERS, which works on fieldable panel panes where workbench moderation does not.
It's...vaguely sort of kind of possible that you could use ERS only for fieldable panel panes while still utilizing workbench moderation for your nodes, though I suspect it will be extremely clunky.
Comment #4
grndlvl CreditAttribution: grndlvl at GollyGood Software for Advomatic commentedI am not sure how this follows ERS's use-case, while, ERS handles the publishing of revisions at specific times it still separates the pane from the Panelized entity.
We have Panelizer working with workbench with the hack/patch from http://drupal.org/node/1402860#comment-7344842 and we def. found out the hard way that these modules do not play together nicely, however, I think this particular use-case could still be valid because it is not dependent on workbench, but rather revisionable entities in general. Especially when you wish to tie an FPP to a specific revision so that when a user edits the FPP on a draft Panelized node via the IPE then the live FPP should not be changed, but yet lives as a separate revision that is specific for the current revision of the Panelized node.
To open up additional dialog I have attached a patch that accomplish this, albeit pretty hackishly.
I also wonder if another approach to this would be to extend the entity field panels content type to allow on the fly editing that respects the revision of the node.
Thanks,
Jonathan
Comment #5
David_Rothstein CreditAttribution: David_Rothstein commentedRight, my understanding is that ERS would be for a situation where you want each pane to have its own independent publishing workflow. But in our case the panes are closely related so the goal is to have them reviewed and published all at once.
In other words, if the page starts off with 4 panes:
The idea is that an editor can add, delete, rearrange, and edit them while working on a draft of the page, such that they wind up (for example) like this:
And then that whole set of changes is pushed live at once.
It does work very nicely with this patch as well as the other one Jonathan mentioned, although both are still a bit hackish.
Since all the changes in the patch here are in a single submit handler, it's possible the right way to do this doesn't involve patching the FPP module at all; another module could swap out the submit handler instead and just deal with it that way. We do require a couple other UI changes for this to behave sensibly (for example, hiding the FPP revision checkbox and forcing it to always be checked) and a separate module could take care of that all at once.
Comment #6
MiroslavBanov CreditAttribution: MiroslavBanov commentedThere is a similar patch that allows use of vuuid here:
https://drupal.org/comment/8428099#comment-8428099
It may be worth looking at it and possibly combining the two patches here.
Comment #7
azinck CreditAttribution: azinck commentedI agree that the logic in #4 is sound. The real challenge here is a UX one. I'd suggest that one way to communicate the difference between reusable and non-reusable panes would be to prohibit the editing of reusable panes in the Panels interface and instead force the user to go to the Fieldable Panels Panes UI to edit those panes.
I'm also thinking we shouldn't allow panes to move between being reusable and non-reusable panes. Once created they should remain as either reusable or non-reusable. If it's necessary to switch from one to the other then the pane should be duplicated. Otherwise you could get into some really confusing UX situations.
Comment #8
azinck CreditAttribution: azinck commentedAlso: I'd love to hear more from merlin about how he sees ERS as addressing this problem. I've given ERS a try and cannot see how it addresses this problem but I may just be dense or there may be another way of approaching this problem that I'm blind to.
Comment #9
EclipseGc CreditAttribution: EclipseGc commentedRerolled against head.
Eclipse
Comment #10
EclipseGc CreditAttribution: EclipseGc commentedI'm actively using this on a site right now and it's performing pretty well, however it has a couple of issues.
1.) Edit->Save a reusable FPP will result in it being removed from the interface.
2.) Cached view of the FPP after save in IPE.
Logic to fix 1.) resulted in a cached FPP because passing the revision id is what causes entity class load() method to bypass cache, so I had to introduce a new type of subtype to support this which is "current:$fpid". There is corresponding logic in _fieldable_panels_panes_load_entity() that selects the max vid of the FPP and passes that along with the load to get the FPP that was just saved. This is definitely still NR but I think we're getting closer to something that's got a fair bit of testing on it and which actually works!
Eclipse
Comment #11
EclipseGc CreditAttribution: EclipseGc commentedInterdiff for those interested
Comment #12
azinck CreditAttribution: azinck commentedI should add that we've been using #4 in production on a very large site for about 10 months now with no problems. We didn't run into the problems described in #10 because we're not using the IPE and we've done a number of alterations to prevent editing reusable FPPs in the Page Manager interface. So for our self-limited use-case it's been working extremely well.
Comment #13
EclipseGc CreditAttribution: EclipseGc commentedIn the spirit of full disclosure, I suggested a similar set of limitations in the system I'm working in, but ultimately the changes required to support the actual feature were pretty minimal. I'm very pleased with the basic approach of this patch.
Comment #14
ramasubbu-drupal CreditAttribution: ramasubbu-drupal commentedHi to everyone,
If we use the "Workbench" module then the node must enable with "Revision". But if we use the "Workflow" module then no need to use the "Revision" option. So the panelized fieldable panel pane will display with any changes and workflow changes on the node.
No need any patches. I have done this flow in my project. Its working fine with workflow and panelizing.
The workflow module is available for Drupal 7 also. Click here to download the workflow module.
Comment #15
ruloweb CreditAttribution: ruloweb commentedThis patch breaks fieldable_panels_panes export using features, because entity_id is containing vid instead of uuid when revision is enabled.
Thanks!
Comment #16
MiroslavBanov CreditAttribution: MiroslavBanov commented@ruloweb
If anyone is interested, I have a patch here that fixes pane load by vuuid:
https://www.drupal.org/node/2101255#comment-8428099
Comment #17
ruloweb CreditAttribution: ruloweb commentedThanks @MiroslavBanov !
I created a patch merging all.
Anyway in order to export fpp using uuid and uuid_features we need also this patch #2499099: FPP vuuid are needed in order to support revisioning
Comment #20
Anonymous (not verified) CreditAttribution: Anonymous commentedOn our site, the #17 patch isn't working with uuid installed. Not sure if it's specifically uuid causing the problem, but the root cause is in PanelsPaneController.class.php's save function.
This is what I changed the line to that checks if it should unset vid or not:
if (!$entity->is_new && $entity->vid) {
It was:
if (!$entity->is_new && !empty($entity->revision) && $entity->vid) {
$entity->revision was blank on our site, I'm not sure if it's because we have uuid or not but our FPP's are definitely revisioning-capable.
This patch should include everything in #17 but with my change. It also has some additional checking for the uuid module later on that could be looked over a little bit more.
Comment #21
EclipseGc CreditAttribution: EclipseGc commentedWhy did this specifically need changing?
Extra spaces.
Is there a specific static you're trying to reset here instead of just all of them?
All in all, this seems pretty sensible to me. It's unfortunate all the module_exists() special casing we've had to do for features like this, but it is what it is.
Eclipse
Comment #22
Anonymous (not verified) CreditAttribution: Anonymous commentedAh, had some spacing from debug statements probably.
I think the static was from the patch originally? I thought that line was odd too, I did not really recall seeing it in the initial patch.
As for #1, I think that line was what was preventing our site from creating the revision since it was always empty for some reason on the entity. I was looking in PHPMyAdmin while testing and when removing it creates 2 revisions properly with uuid's set. With just #17 applied, it kept editing only the FPP that existed. Maybe there's some other setting on our site preventing FPP revisions? I don't see the revision UI element when editing FPP's on our IPE popup but can confirm it's set to 1 on the form element.
Comment #23
MiroslavBanov CreditAttribution: MiroslavBanov commented@vilepickle
#1 $entity->revision comes from the UI element and is a flag for whether to create a new revision or not. I don't understand why you have the problem and can't reproduce it.
#3 The drupal_static_reset() is not part of the original patch.
Edit: I feel like you are experiencing a separate issue, aside from this one, but maybe I'm wrong.
Comment #24
EclipseGc CreditAttribution: EclipseGc commented@vilepickle
I agree with MiroslavBanov here. The $entity->revision is just a boolean statement for the entity system about whether to create a new revision or not. If that is NOT set to true, you have another problem (which is likely part of the greater issue).
The drupal static is not part of the original patch. It's good practice to provide interdiffs between patches which would also reveal these sorts of issues.
Eclipse
Comment #25
Anonymous (not verified) CreditAttribution: Anonymous commentedSo I agree that my patch is not for a generic use-case looking at it closer. We have no UI element for revisions on either /admin/structure/fieldable-panels-panes/*/add or the IPE popup. Is there some option for turning them on or off on FPP's? I'm logged in as an administrator and revisions are enabled on content types.
Comment #26
MiroslavBanov CreditAttribution: MiroslavBanov commented@vilepickle
Please open a separate ticket for that.
I think we should re-post and review patch #17, because it focused on this issue.
Comment #27
Anonymous (not verified) CreditAttribution: Anonymous commentedYeah, sorry about that. We had a feature with a custom form alter from when our site was built removing those elements from the form. Go figure.
Comment #28
heddnComment #29
MiroslavBanov CreditAttribution: MiroslavBanov commented#17 still needs review. Or maybe it can be re-uploaded without change, just to be the last patch.
Comment #33
heddnWith the changes in _fieldable_panels_panes_load_entity(), I had to move a hunk around to fieldable_panels_panes_load_from_subtype_force. No interdiff as this is essentially a re-roll.
Comment #34
heddnShould this block commit? I'm seeing that a new revision for the FPP gets created on *every* save. Nothing in here checks if the FPP is identical to the previous revision. I'd prefer some type of comparison so I don't clutter up field revision and fpp_revision tables. It was brought to my attention that is actually the case, even before this patch. So I should probably open a follow-up to address.
To reproduce:
Comment #35
heddnComment #36
heddnIgnore #34. This is due to a form alter that hides and checks the create new revision checkbox.
Comment #37
DamienMcKenna@heddn: Which patch did you reroll from - #17 or a different one?
@EclipseGc: Could you please confirm that patch #33 still works for you? Thanks.
Comment #38
heddnre #37: This was a re-roll of #17.
Comment #39
MiroslavBanov CreditAttribution: MiroslavBanov commented@heddn
It's quite clearly not a re-roll of #17. It is a re-roll of #20. #20 introduces three changes, all of them questioned in #21, further discussed #22 through #27, and still without justification. #33 is a straight re-roll of #20 and includes all the changes from #20. I am re-uploading #17 without any modification.
Comment #40
DamienMcKennaComment #42
heddnre #39, good catch. I had intended to grab #17, but apparently I didn't. This should fix that, and the re-roll too.
Comment #43
saltednutHas anyone tested this latest patch with UUID module turned on? I am finding that it functions as expected up until I turn on UUID. At that point I get the dreaded
Placeholder for empty or inaccessible "Deleted/removed entity pane"
message in place of the FPP inserted.Comment #44
saltednutOk I read the whole issue and found that the patch from #33 to be a stop gap for us. Guess we should address the issues brought up in #17 either in a followup, or an issue for UUID or as part of the next patch if we can determine there's something we can do on the FPP side of things and thats the correct thing to do.
Comment #45
MiroslavBanov CreditAttribution: MiroslavBanov commented@brantwynn,
If I have to guess, there is one legitimate change in #20, which you can try:
Comment #46
mairi CreditAttribution: mairi commentedI am uploading an updated version of the patch at #42, along with an interdiff to show what has been changed. This patch resolves issues with FPP revisions for us - using fieldable_panels_panes 7.x-1.7 with uuid. We also have organic groups & workbench_moderation in play, but they don't seem to be a factor in this particular scenario.
Changes from the previous patch #42 are as follows:
Interdiff attached to show changes between patches #42 and #46.
Comment #47
danieltome CreditAttribution: danieltome commentedHi @mairi,
I applied the interdiff patch from 42-46 on a brand new lightning alpha install, and it is working well.
FPP Panels are showing correctly on draft and published versions.
There is one issue with media though.
Steps to reproduce:
1) Create new landing page panel and add a media panel and upload an image.
2) Publish page.
3) Create new draft, then edit media panel and change the image to a different one.
Issue:
Latest image is shown on draft and published nodes.
Expected result:
Draft node to show new image, published node to show the previous image.
Comment #48
saltednut@danieltome's issue sounds like it is related to the file entity that is attached to a fieldable panels pane field. It may be out of scope for this particular issue to solve keeping versions of file entities working with FPP revisions. If anyone else is seeing this or knows of a related issue, perhaps we can cross reference.
Comment #49
MiroslavBanov CreditAttribution: MiroslavBanov commentedI think the issue in #46 is integration of panelizer and workbench moderation. The patches help fixing uuid and FPP revisioning, but do not completely resolve original issue.
Maybe need to split or do follow-up?Actually the other issues are probably for other modules to resolve.Comment #50
mairi CreditAttribution: mairi commented@danieltome Sorry you're still having issues after that latest patch. We are not using media panels, or the lightning distribution so I'd need to get that locally to investigate, which I won't have time for this week. It does sound like a 'related but different' issue to me, but wouldn't want to dismiss it as out of scope for this patch without looking at it more closely. As @brantwynn suggests, perhaps we can cross reference if there are related issues.
I can say that we have had a lot of trouble getting fpp revisions & workbench_moderation to play nicely together, which seems to be a common experience! This patch and one of the ones from workbench_moderation (https://www.drupal.org/node/1402860) are working for us. I don't know if the latter would resolve your issue. I need to review our use of that patch actually as it's now ahead of where it was when we started using it. Ah, the joys of fpp/panelizer/workbench_moderation! :-)
Comment #51
danieltome CreditAttribution: danieltome commentedThanks @mairi, I think @brantwynn is right, in regards to #48 and that it is related to file entities, and the way the replace image is done.
I can raise that in another issue. re: file_entity, fpp and workbench_moderation.
I'm happy this patch is moving forward and it is working well with all other panels.
What do we need to do to get this patch release into the module?
Comment #52
mairi CreditAttribution: mairi commentedPatch re-roll to resolve offsets with fieldable_panels_panes v7.x-1.8.
Just to reiterate @danieltome's question - it would be good to know what's needed to get this into the module.
Comment #53
DamienMcKennaJust to mention it, I am seriously considering closing this as a "works by design" and recommending people use Paragraph Panes instead. Has anyone considered that module for doing per-display revision-aware changes?
Comment #54
azinck CreditAttribution: azinck commentedThat's a very promising module @DamienMcKenna. Thanks for the pointer. I agree that it would solve these problems well. FPP would remain to serve use-cases where you'd want to re-use panes.
Comment #55
DamienMcKenna@azinck: That's exactly what I'm thinking.
Comment #56
saltednutI am a bit torn on #53. While I have seen a lot of blood, sweat and tears in this issue and others regarding this I don't want our collective pride to cloud judgement. Maybe this issue is a bridge too far or maybe its not.
At the same time everyone is really mostly focused on D8 and there hasn't been much review going on here. @mairi posts a new patch and @DamienMcKenna's response is kind of short and dismissive of the entire issue. Which I mean, maybe doesn't bother her, but I personally would hate to work on a patch so hard to have the maintainer completely ignore it and not even answer the question at hand, instead sort of kicking rocks around and telling everyone we're wrong for trying to work this way...
So for distros that use these patches (I know Lightning 7.x uses it, not sure who else besides that and DF) it would be a pretty sad day if this was completely abandoned. I don't have the resources to completely re-architect the 7.x branches of those distros. And there's no way with Panopoly and Lightning both promoting FPP that we could somehow transform everyone over to users of this new module recommended. People are using FPP in these distros and if they need moderation for panelized pages that include FPP, they are in trouble.
We're holding on to this, hoping we can finally get it working for the sake of the slim number of users that have Lightning 7.x in production. They are definitely affected by this and won't have the luxury of complete re-architecture either.
Comment #57
azinck CreditAttribution: azinck commentedI hear you Brantwynn. I think this patch, as-is, solves the basic technical problem, but that there's a major UX problem with the notion of reusable panes and how that interacts with revisioning such that I really think this solution is only a half-measure as it stands. On the major site where we do use this patch we've had to write a decent bit of custom code to make our site behave as I outlined in #12. And I don't know that our approach could be suitably generalized to serve all use-cases.
I've not checked out Lightning. Does it solve the UX problem?
Comment #58
DamienMcKennabrantwynn: You are correct, I was definitely overly dismissive in my comment.
I am very appreciative of all of the work that went into this patch, so a huge thanks to mairi, EclipseGc, heddn, ruloweb, MiroslavBanov, David_Rothstein, vilepickle and grndlvl! I was too hasty with my response and I sincerely apologize for that.
I will test this patch out (probably next week), and given it's already in use in the wild so much (distros? ok, I didn't think about that) will likely commit it to avoid causing problems for these sites.
That said, the goal of this functionality matches what Paragraph Panes already does, only it does so without the associated problems of having a separate physical entity object stored in the database. This then allows the data to be exported along with the rest of the Panels/Panelizer display via Features, which is another often-requested feature for FPP.
The situation we're in is analogous to what happened with EntityReferences and revision tracking - while some of us were working on shoehorning revisions into EntityReferences (#1837650: Allow referencing a specific revision ID) someone else came up with the Paragraphs module, which is almost exactly what most of us wanted in the first place (though I'd have preferred it to be more of a generic entityreference module that could work with all entities).
What I'll probably do is commit this and add documentation to say this works but explain that most sites will probably want what they get from Paragraph Panes instead because of the other benefits.
And again, I am sorry for how I handled my initial response.
Comment #59
mairi CreditAttribution: mairi commented@DamienMcKenna Please don't worry about your initial response - I completely appreciate Paragraph Panes is a better alternative & can totally understand your initial suggestion. Unfortunately, as @brantwynn suggests, we're not in a position to re-architect what we have, which was embarked upon when Paragraph Panes wasn't around. If this patch isn't accepted, we will at least for a while have to continue re-rolling & re-applying it on each release of FPP. Having the patch committed & docs updated to point to Paragraph Panes would be a great result for me personally since I'm usually the one doing the re-rolling & testing :-) Thanks and fingers crossed for a positive outcome! The patch has been in use on our production site for over a year now & working well, although we don't have re-usable panes enabled.
Comment #60
saltednut@DamienMcKenna: I should apologize, as my post yesterday should have been much nicer. Sorry about that. Thank you for the extremely well thought out response on this issue. It is a sticky one for distros but you are right 100% about the problem this causes re: multiple asynchronous entities. We already see this issue with blocks and panelizer in D8 too, but maybe that is to a lesser extent.
My take-away from this whole thing is that there isn't much we can do for the UX issues @azinck mentions. Lightning in D7 and D8 attempt to subvert the problem by turning off the Panels IPE when the user is viewing the currently published node. I am not sure if that approach is sufficient for everyone but it seems like it will prevent some problems that could occur - though not all.
Thanks again for taking the time to look into this and a big ++ to everyone that has been keeping this patch alive!
Comment #61
DamienMcKennaRerolled, and includes a tiny fix to some comments.
Comment #62
DamienMcKennaIs it possible that the logic for detecting whether to the vid or fpid in _fieldable_panels_panes_custom_content_type() was incorrect? Shouldn't it track the vid if the FPP is reusable and the fpid if it is not reusable? In my testing it was not tracking the vid for a reusable FPP.
Comment #63
DamienMcKennaThere was one more occurrence of the same logic.
Comment #64
DamienMcKennaI wonder if maybe the extra revision tracking should be toggleable via a variable, with it set to "off" by default (Lightning could add an update script to turn it "on")?
Comment #65
DamienMcKennaSome updates to the comments.
Comment #66
saltednutre #64, thats totally fine. We can ship with a variable_set during hook_install and also provide an update hook to set the new variable for existing users.
Comment #67
azinck CreditAttribution: azinck commentedRe #62:
For my use-cases I want to track the vid only for non-reusable panels and *not* track the vid for reusable panels.
Tracking the vid for non-reusable panels allows you to edit the panel while editing the node, and then have different FPP revisions embedded on different node revisions. When you edit an FPP on a node (or other entity) revision you don't tend to want those FPP changes to spread to all places that FPP is embedded; you want the edits to be specific to the revision of the parent entity on which you're doing the editing. In our use-case we've hard-coded the "create new revision" checkbox to always be true when editing non-reusable FPPs so that you can never accidentally step on the toes of some other parent entity revision where you've embedded the same FPP. Something like this is often the desired result:
But reusable FPPs are rather a different story altogether. The whole point of those is to be able to make a change in one place and have it propogate to everywhere the FPP is embedded. So those should not be embedded as a specific revision, instead always showing the current revision of the FPP.
That said, communicating all of this nuance to the user is tricky and is why we made the modifications I mentioned in comment 12.
Comment #68
DamienMcKennaThis needs a little more work to accommodate a global setting, which could be added to the new settings page which was just added via #1910934: Expose Fieldable Panels Panes as blocks.
Comment #69
DamienMcKennaI also think we need to add something to the README.txt to describe the workflow change, and we'll need a changenotice too. That said, I think we can get it into the next release :)
Comment #70
MiroslavBanov CreditAttribution: MiroslavBanov as a volunteer commentedPer #67, what we want is a checkbox to "Force revision on non-reusable panes". While I agree and would use the setting, can't we make it a follow-up issue?
Comment #71
DamienMcKenna@MiroslavBanov: Yes, that can be a follow-up issue.
Comment #72
azinck CreditAttribution: azinck commented#70 and #71: I was just trying to address the question raised in #62. In #62 and 63 Damien flipped the logic to embed a specific revision only if the FPP is reusable. I am trying to argue that the old logic was correct and that Damien's new logic is wrong.
Comment #73
MiroslavBanov CreditAttribution: MiroslavBanov as a volunteer commentedHm, I agree - the flipped logic is not correct.
Should be changed. Let's change the
!empty()
toempty()
, and commit to get this out of the way.And then to follow-up (in the same issue or in different one) and figure out what to do with configuration. Maybe #64 and #67 can be controlled by the same variable? But need more people to chime in on it.
Comment #74
DamienMcKennaOk, life happened and one of our clients requested this, so I'll be finishing it off tomorrow. :-)
Comment #75
DamienMcKennaThis adds a new setting that controls the revisions functionality via a new variable ("fpp_revision_locking").
Comment #76
DamienMcKennaNote - I haven't tested it yet, I'll do that in the morning.
Comment #77
saltednutUpdated the dev version of Lightning Features to include FPP (dev HEAD) w/ the new patch. Everything is working fine but I don't have automated tests working on our end yet to ensure no regressions. That said, we are working on that. I did not want to hold anyone up and Damien had asked me on IRC to look at this. All of my manual tests seem to be working fine.
Test 1. Lightning 7.x-1.x-dev (non-reusable panes)
Test 2. Lightning 7.x-1.x-dev (reusable panes)
4 w00ts all around, which by my count seems to cover both re-usable and particular revisions of a FPP to be placed in a panelized page.
Comment #78
MiroslavBanov CreditAttribution: MiroslavBanov as a volunteer commentedThis setting
fpp_revision_locking
is explained as:Should pane changes create new revisions?
But what it actually does is control if panes content should target pane revision id, or the pane entity id. The term "locking" is not very informative by itself.
The checkbox "Create new revision" still controls if a new revision is created.
Comment #79
MiroslavBanov CreditAttribution: MiroslavBanov as a volunteer commentedTrying a better description of the setting.
Comment #80
DamienMcKenna@MiroslavBanov: Did you try testing the functionality itself?
Comment #81
MiroslavBanov CreditAttribution: MiroslavBanov as a volunteer commentedI have been using early versions of this patch for a while, in order to fix this issue #2101255: Updating existing FPP-pane with media field renders wrong in preview. I use it on a panel page that is not revision-enabled, so most of the issues resolved here only affect me in unlikely corner-cases. I did test this on a clean install, and validated some assumptions, but I can't be as detailed as brantwynn.
I agree with the changes. I am just unsure about the naming of the configuration, because it is hard to explain the implications. I did change it and I still don't like it. Feel free to reject the changes in #79.
Comment #82
saltednutWe now have some automated testing via Lightning that shows this patch working. Video attached (sorry a little blurry, but you should get the gist).
Shows use Test 1 from #77
This is as close to RTBC I can get, but I will suggest we await @azinck for confirmation that his use cases are covered.
Comment #83
azinck CreditAttribution: azinck commentedThanks @brantwynn. I've been using an early version of this patch for a long time. I'll test the newer version this afternoon and report back.
I don't want to open a huge can of worms, but I'd like to suggest that, if someone has chosen to "lock" the pane, that perhaps we should also by default create a new revision of the FPP for them any time they edit it. I'm not clear on all the ways people are using this functionality so maybe I'm off-base but, to me, this seems a better default.
Comment #84
azinck CreditAttribution: azinck commentedI just tested #79 and it is working correctly for me. I agree with @MiroslavBanov that the language on the settings page in #75 is very confusing. The language in #79 is better; not perfect, but at least better.
I'll mark #79 as RTBC. I still would love to see UX improvements (per my comment in #83) but recognize that this functionality is being used in a wide variety of ways and #79 is a definite step in the right direction.
Thanks to Damien and Miroslav for your contributions.
Comment #85
DamienMcKenna@azinck: Your idea of auto-creating a new revision is a pretty good one, maybe it's worth spinning off another issue to cover that?
Comment #86
sluceroThere are no functional changes here from #79. This is just a re-roll of the patch with a couple of typo corrections in the admin interface.
Comment #87
azinck CreditAttribution: azinck commentedI've taken out #2695499: Improve UX when working with FPP revisions as a follow-up to try to deal with some of the UX problems coming from this patch.
Comment #88
DamienMcKenna@azinck: Thanks.
Comment #89
azinck CreditAttribution: azinck commentedComment #91
DamienMcKennaCommitted! Thank you all for the work and patience on this. We'll be following up with additional issues to improve the UX, and working on documentation to make it clear what happens, but this is a huge step forwards for sites that need it.
Comment #93
saltednutSo for some reason, our automated tests are now failing since updating to the latest commit.
-- https://travis-ci.org/acquia/lightning-features#L1148
My manual tests (same as test case 1 from #77) are now failing too.
The point of failure is the actual retention of the revision -- e.g. I edit the FPP in Draft mode and the FPP is updated on both the Published revision and the Draft, both when the 'make this reusable' checkbox is checked and when its not checked.
Will try to get to the bottom of this since literally nothing has changed in the patches based on the interdiff.
The last patch we had applied working was from #75 applied to revision a067f9b
Comment #94
saltednutOk, ignore #93 -- I am realizing now, in my earlier tests, something must have been off - not sure what, but we need to explicitly set fpp_revision_locking to 'lock' in order for this to work as expected. I went through the patch from #75 we had been using line by line and found that variable. Not sure why the tests managed to pass before given the variable should always have been set to 'legacy' by default. I think we're good. False alarm!
-- https://travis-ci.org/acquia/lightning-features/builds/119307183#L1148
Comment #95
sluceroFor what it's worth, I just double-checked the patch re-uploaded in #86 against the original in #75. I'm not seeing anything changed that's not represented in the interdiff.
Comment #96
DamienMcKennaFYI please follow #2695499: Improve UX when working with FPP revisions while we refine this functionality.
Comment #98
PolI think this commit introduced a problem with Entity Translation.
See #2820177: Revision ID breaks translation workflow.