Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Block concurrent editing #543

Closed
willguv opened this issue Apr 28, 2023 · 15 comments · Fixed by #580
Closed

Block concurrent editing #543

willguv opened this issue Apr 28, 2023 · 15 comments · Fixed by #580
Assignees
Labels

Comments

@willguv
Copy link
Member

willguv commented Apr 28, 2023

This has been raised by @keelanfh and others. I'll confirm with the content group this need is common

Two users should not be able to edit content at the same time

Ideally...

  • /admin/content and other list pages (eg /admin/content/alert-banner) should show content that is locked and name of user that's editing)

  • there should be some way to override a lock

This module looks like a good fit: https://www.drupal.org/project/content_lock

@willguv
Copy link
Member Author

willguv commented May 10, 2023

Andy recommends the sub module that clears out locks on chron (should be already included). Also a permission that be assigned to editors to break locks

Stephen: there's an admin overhead so maybe point to it rather than include for all? Can play havoc with autosaving

Keelan: Remembers 2 year old content locks in previous system, one admin because people had left

Finn: difference between installing and making easy to configure, vs we recommend. Check with content group

@willguv
Copy link
Member Author

willguv commented May 23, 2023

Content group 23 May says this is a good idea. General feeling was that this feature should really be included

@stephen-cox could this be installed on test please

@stephen-cox stephen-cox self-assigned this May 24, 2023
@stephen-cox
Copy link
Member

@willguv I've deployed Content Lock to the test site: https://test.localgovdrupal.org

@stephen-cox stephen-cox removed their assignment May 25, 2023
@willguv
Copy link
Member Author

willguv commented Jun 7, 2023

Having a look now, thanks @stephen-cox. Posting any issues as I see them

  1. Status messages need a bit of work. Don't think any headings are needed

Image

Locked by [name] [time] ago

Image

Content locked. Remember to save or unlock before you leave

Image

Image

  1. First article in this list is being edited and so is locked. It's not clear without clicking on the article

Image

@willguv
Copy link
Member Author

willguv commented Jun 7, 2023

At BG group today we agreed:

  1. Add code to distribution, default off. In core rather than workflow

  2. Let's see how easy it is to:

  • configure status messages @stephen-cox to find out
  • add "locked by" messages to /content view

Consider including "content lock time out" sub module

  1. Write "how to" documentation. Consider "how to extend LGD" section

@stephen-cox stephen-cox self-assigned this Jun 19, 2023
@willguv
Copy link
Member Author

willguv commented Jun 29, 2023

@stephen-cox am really keen to get this done in the next month, in time for product update 3

Could you have a look at the points in (ii) above please? Also do you have 10 mins to show me the content lock time out controls?

Thanks, Will

@stephen-cox
Copy link
Member

@willguv There's a PR for this here #580.

It just installs the Content Lock module and will do this for any sites updating LGD as well as new installs.

do you have 10 mins to show me the content lock time out controls?

Happy to show you them, but they're very simple. You can see them here: https://test.localgovdrupal.org/admin/config/content/content_lock/timeout

image

On the questions:

It's not straightforward to override the messages displayed by this module. They are hardcoded into the module in a Drupal service. It's may be possible to inject our own service that extends the ContentLock class and changes these messages. We could also just patch the module, but for either option the Content Lock module could introduce changes that break our changes and so maintaining them could be a pain.

It is possible to add the user who locked the content to the main content overview, but I would be wary about changing this. We certainly don't want to do this for existing installs. Changing this for new installs is possible, but would be a little complex. The module provides a locked content view which includes this sort if information (https://test.localgovdrupal.org/admin/content/locked-content). Would making this more visible by adding it to the content menu and tabs be sufficient?

@willguv
Copy link
Member Author

willguv commented Jul 3, 2023

@stephen-cox thanks for doing this

I don't want to introduce customisations that we need to keep an eye on. Instead is it possible to submit a PR to the module maintainer that allows custom wording to be added? If that's not lots of effort I'd be keen to look at that

Adding the locked content view: that's a good compromise. Could it go in the oveview/ moderated content/ approvals dashboard line at the end? Label would be "Locked"

@willguv
Copy link
Member Author

willguv commented Jul 5, 2023

Hi @stephen-cox any thoughts on the above please? Would be good to know the plan before today's backlog group - many thanks

@stephen-cox
Copy link
Member

@willguv Adding the ability to edit messages will be quite time consuming as there will need to be an admin UI for this that supports tokens (so things like usernames can be added).

Much easier to add some menu items for the view when it's enabled.

@willguv
Copy link
Member Author

willguv commented Jul 11, 2023

Add "Locked" to the content sub tabs ie the line with overview on

@stephen-cox
Copy link
Member

@willguv This is now on the test site for you to have a look at: https://test.localgovdrupal.org/admin/content/locked-content

I've create a wrapper module (localgov_content_lock) which enables and configures the content_lock and content_lock_timeout modules. There's some basic docs here https://github.com/localgovdrupal/localgov/blob/feature/2.x/543-content-locl/modules/localgov_content_lock/README.md

@willguv
Copy link
Member Author

willguv commented Aug 15, 2023

@stephen-cox I'm getting page not found for https://test.localgovdrupal.org/admin/content/locked-content

@stephen-cox
Copy link
Member

@willguv I've enabled the LocalGov Content Lock module. As the test site gets reset every night this will have to be done whenever you want to test things it

@willguv
Copy link
Member Author

willguv commented Aug 15, 2023

@stephen-cox just tried it, looks great thank you

I would still like to tidy up the notification messages as they're very verbose, but I guess we'll have to let that go

Could we a message to the module issue queue about this?

ekes added a commit that referenced this issue Aug 24, 2023
Squashed commit of the following:

commit c2ff5d9
Merge: 90d3d0f 5dc2608
Author: ekes <ekes@iskra.net>
Date:   Thu Aug 24 17:23:59 2023 +0200

    Merge branch '2.x' into feature/2.x/543-content-locl

commit 90d3d0f
Author: Stephen Cox <stephen@agile.coop>
Date:   Mon Aug 14 13:37:24 2023 +0100

    Don't configure content locking if syncing from config

commit 2ca2289
Merge: a6b1b2b 7711669
Author: Stephen Cox <stephen@agile.coop>
Date:   Mon Aug 14 13:18:51 2023 +0100

    Merge branch '2.x' into feature/2.x/543-content-locl

commit a6b1b2b
Author: Stephen Cox <stephen@agile.coop>
Date:   Mon Aug 14 13:12:39 2023 +0100

    Coding standards fixes

commit 73fabb3
Author: Stephen Cox <stephen@agile.coop>
Date:   Mon Aug 14 13:06:59 2023 +0100

    Added test for content lock configuration

commit 5328995
Author: Stephen Cox <stephen@agile.coop>
Date:   Mon Aug 14 12:43:32 2023 +0100

    Added localgov_content_lock module to customise content lock settings #543

commit 993d00a
Author: Stephen Cox <stephen@agile.coop>
Date:   Fri Jun 30 13:09:17 2023 +0100

    Add but don't enable the Content Lock module #543
@willguv willguv added the #6 label Oct 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Development

Successfully merging a pull request may close this issue.

2 participants