Page made read-only for default admin

[raissanorth]

I followed the instructions in the userguide re setting up workflows to my best understanding and ran into a few issues:

When I tried to 'apply for approval' aka initiate the apply and review process as a content author via the CMS buttons, first an ‘internal server error’ and then, after trying again, a ‘forbidden’ toast message appeared.

These messages are 1) unexpected and 2) not descriptive enough to a user.

After a reload, the page I edited as a content author is read-only and the save and apply for approval buttons disappeared. This is expected, however I somehow managed to make the page read-only for the default admin as well. This is not ideal, given that I cannot take the workflow off that particular page anymore.

This appears to be a regression in CWP 2.0, as I cannot reproduce this behaviour on CWP 1.x

Or is this intended? I assumed an administrator should always be able to edit and change settings to any page.

I tested

symbiote/silverstripe-advancedworkflow    dev-master da6b3a1
silverstripe/cms                          4.1.x-dev c6af885

against

symbiote/silverstripe-advancedworkflow    4.0.1             
silverstripe/cms                          3.6.5           

Also, the user group ‘Content Publishers’ I made - following the docs - is denied access to the CMS altogether.

[NightJar]

This could be an unintended side effect of #333 coupled with some misbehaviour as per the likes of #359

[phptek]

@raissanorth can you look at your dev-tools' "Network" tab, for XHRRequests? I have exactly the same problem moving on past an approval stage becuase I was logged in as default admin. For me, there's a swiftmailer Invalid Address error, which is logical becuase the "Default Admin" user doesn't have a proper email address ("admin") is used as the username - never understood that one myself.

What I have always done with this module when testing it, is to create two users (for simple 2-step workflows) and login to Chrome with one, and Firefox with the other. Doing this you have a). de-facto, valid email addresses to test with b). You can eliminate being "Admin" and being able to do everything, so you can properly test permiissions and c). You don't get confused and go "I'm logged in as "admin"..who is that again, the author or approver?"

1). "Author" (e.g. "Alice Author" and use my own email address with the "[email protected]" syntax
2). "Approver" (e.g. "Alan Approver" and use my own email address with the "[email protected]" syntax

[NightJar]

Sounds exactly my experience with #359 @phptek - thanks for updating :)

[hp7]

Experiencing the same behavior making the module unusable.
On SS 4.1.0
Module used is the Dev-Master
I get the Errors and the user group ‘Content Publishers’ is denied access to the CMS pages. I also followed the docs. Maybe along with this bug the docs are wrong too

[robbieaverill]

@hp7 which users are you using in the case it's breaking for you? I.e. what are they email address formats (you can obfuscate)

[NightJar]

I've just close #359 as a duplicate of this issue, as per @phptek 's comment earlier - I've confirmed it to be the case and the reason for the issue described in this issue's OP.
Using the (default) Unique ID field of Member.Email as a username (instead of a valid email address) will throw an exception from Swiftmailer, interrupting execution and resulting in this 'lock in' to what should be a transitional state.

I questioned whether or not this should actually be considered in core, since it used to work but now doesn't. But of course that's acceptable in a major version change, and requesting core to "send an email to " - it's perfectly reasonable to throw an exception (...as opposed to asking to "Notify user" and core choosing to email an invalid address).
advancedworkflow should check email validity before attempting to send/notify.

[robbieaverill]

advancedworkflow should check email validity before attempting to send/notify.

👍

[robbieaverill]

Reproduced. Wow - what a set up process!

[robbieaverill]

I'm making a PR to catch invalid email address formats and ignore them, rather than break the processing for the approval workflow and/or any other members in the group that needs to be notified in NotifyUsersWorkflowAction.

The read only CMS part seems expected to me because of the way the workflow is configured

[robbieaverill]

PR at #370

[raissanorth]

@robbieaverill Re:

The read only CMS part seems expected to me because of the way the workflow is configured

If you think really think so, then we'd need to at least update the following bit in https://github.com/symbiote/silverstripe-advancedworkflow/blob/master/docs/en/userguide/setting-up-workflows.md

Note: Any user with "full administrative rights" set will see all Workflow action buttons in the CMS, plus they will have the ability to draft and publish content regardless of whether or not a workflow is configured on a page.

[robbieaverill]

Ha, touché

[phptek]

@raissanorth note my previous comments viz taking 2m to set up 2 users with different workflow perms and use 2 different browsers to test. The latter is not for any technical reason; rather to ensure you keep what sanity you have left after working with "advanced" workflow ;-)

[NightJar]

#370 is merged, although it mentions only addressing half of this issue. What is the outstanding half? Tests?

[robbieaverill]

although it mentions only addressing half of this issue. What is the outstanding half?

The other half is that a CMS admin can be denied ability to edit a page when a workflow is in progress.

I can't see anywhere in the code for "can the current user edit this record" that says "admins can always edit this record," so either the code or the docs are wrong currently, given that the docs say "Note: Any user with "full administrative rights" set will see all Workflow action buttons in the CMS, plus they will have the ability to draft and publish content regardless of whether or not a workflow is configured on a page."

[robbieaverill]

Second half fix at #371

[NightJar]

I don't think the second half was actually to do with edit permissions, rather that a workflow was stuck in the middle of a transition and had no path forward. This should have been solved with the first half as such, but I didn't see the harm in the second half either.

The only query I have is whether or not it's obvious for an admin that a workflow is in progress and a page probably shouldn't be updated (as opposed to blocking such an update with read-only views).

[robbieaverill]

The only query I have is whether or not it's obvious for an admin that a workflow is in progress

I don't think it is obvious, nor has it been in the past. If we want to look into this we'd need a new issue to investigate potential UX updates

[NightJar]

I think it's worth considering perhaps a simple bootstrap alert in the least, since this is functionality we've just explicitly introduced. How do you feel about opening a separate issue for it in the least?

[robbieaverill]

We haven't changed anything that wasn't originally in place in SilverStripe 3 or supposed to be happening as far as I could tell.

• Admins can always edit pages regardless of workflow status -> broken in SS4 but noted in the docs as how it should behave
• Emails won't get sent to invalid email addresses and break the workflow -> regression from SS3 to SS4

[NightJar]

broken in SS4 but noted in the docs as how it should behave

OK, that's what I wanted to hear I guess. I wasn't aware (of the first bit at least), thanks!

[davejtoews]

@NightJar, @robbieaverill I'm a little confused why this issue is closed.

I just ran into this problem on SS4.4 with advanced workflow 5.2. Hitting errors when using a default admin user without a proper email address ( a common pattern outlined in the SS docs). I feel like there should at least be mention of requiring an email address or perhaps setting SS_SEND_ALL_EMAILS_TO in the Advanced Workflow docs to help people avoid this problem in dev environments.

[robbieaverill]

We can reopen and reinvestigate. The issue was closed because we thought we’d fixed it 🙂