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

Set a default for events to email in User Action Log Options #38439

Merged
merged 2 commits into from
Aug 12, 2022

Conversation

roland-d
Copy link
Contributor

Summary of Changes

When you create a new super user and this super user logs in and tries to save their profile without changing the options on the User Action Log Options the save will fail with this error:
image

One way around this is to set Email Notifications to Yes and select 1 or more extensions and save the profile. After that set the Email Notifications to No and save it again. The error will be gone but this is not user friendly.

The solution is to set a default value for the action logs extensions because then a value will be saved but that does not matter because the Email Notifications defaults to No so nothing will be set. If a user decides to set this to Yes, they need to select which extensions they want anyway.

Testing Instructions

  1. Create a new super user account
  2. Login with the new super user account
  3. Click on User Menu -> Edit Account
  4. Click on Save
  5. The error as shown above appears
  6. Apply patch
  7. Click on Save
  8. The account is saved successfully

Actual result BEFORE applying this Pull Request

User account cannot be saved

Expected result AFTER applying this Pull Request

User account can be saved

Documentation Changes Required

None

Signed-off-by: Roland Dalmulder <[email protected]>
@bayareajenn
Copy link

I have tested this item ✅ successfully on 6471330

Works great. Thanks, Roland.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/38439.

@viocassel
Copy link
Contributor

I have tested this item ✅ successfully on 6471330


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/38439.

@Quy Quy added the RTC This Pull Request is Ready To Commit label Aug 11, 2022
@fancyFranci fancyFranci merged commit 0110c04 into joomla:4.2-dev Aug 12, 2022
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Aug 12, 2022
@heelc29
Copy link
Contributor

heelc29 commented Aug 12, 2022

@roland-d See for changed behavior #37840

@brianteeman
Copy link
Contributor

This is obviously not a correct fix. You have put a band aid on the issue that only resolves it partly.

@brianteeman
Copy link
Contributor

brianteeman commented Aug 12, 2022

animation removed - searching for an option to make it loop once only

@nikosdion
Copy link
Contributor

@brianteeman It is the correct fix but you are also right that this is a band-aid. The real problem is two-fold.

  1. Something about initialising fields which return array when you try to save empty array
  2. Something about not being able to create a custom handler in plugins

I could explain more if it wasn't for that forever looping bloody animation you have put in your comment!

Please, for the love of whatever deity you believe in, DO NOT POST INLINE LOOPING GIFs IN YOUR COMMENTS! For someone who allegedly cares so much about accessibility you are being actively hostile to people with ADHD. That bloody animation is catching my attention and won't let me think.

I know exactly the problem, I have seen it myself in Admin Tools' Web Application Firewall and I could tell you exactly why it happens if you had not made this page inaccessible to me and other people with ADHD.

@brianteeman
Copy link
Contributor

gif removed while I recompi;le the app - sorry

@nikosdion
Copy link
Contributor

Thank you! For the GIF, you can drag'n'drop it to GitHub and after it's done change the inline image tag to a link. This way we can still see the animation, in a new window, then choose to close the window. What you were doing is not a problem on desktop — I can just make the edit area longer to hide the animation — but on mobile which forces me into the GitHub app it's frustrating. I'd normally use the accessibility option for less animation but this disables other visual cues on iOS which makes it just as much of a problem as it is a solution. Ugh.

OK, back to the issue at hand. The problem is that Joomla's form fields apply the default value when the configured value is "empty". However, PHP considers the following to be "empty": null, 0, array(), ''.

When you have a field which results in an array Joomla cannot distinguish between the uninitialised state and a deliberately saved empty array. To change that behaviour we'd have to return NULL when the option is uninitialised. However, that would be a substantial b/c break since all code written from at Joomla 1.6 onwards assumes that the uninitialised state of an array-returning form field is an empty array. That's why you typically see a no-op option (e.g. a module position "None") in these kind of multi-select form fields.

So looping back to what @roland-d did here, it's not wrong. It's the right way to do it. Yes, it's a band-aid because then way Joomla behaves makes no selection an impossibility UNLESS you have something like <option value="">No Selection</option> in the XML form and you, the user select the No Selection no-op option instead of clearing out all selections before saving the user profile.

Yeah. I know. This behaviour has been the source of many a complaint and frustrated user in Admin Tools' Configure WAF page.

@brianteeman
Copy link
Contributor

brianteeman commented Aug 12, 2022

That's why you typically see a no-op option (e.g. a module position "None") in these kind of multi-select form fields.

This is what I would consider to be the correct fix and why arbitrarily setting it com_content is not a fix - Especially annoying/confusing when the unselected items come back on save :(

@nikosdion
Copy link
Contributor

@brianteeman I agree, on the basis that this is what is the accepted typical solution in Joomla even though it's problematic user experience (the user has to be taught that they need to make a special selection which means no selection is made instead of just removing all selected items, something that does boggle my mind every time I have to it; it's so unnatural).

I would say that you should do a PR to add this missing no-op option here.

@roland-d roland-d deleted the feature/default-user-actions-log branch August 12, 2022 19:07
@zero-24 zero-24 added this to the Joomla! 4.2.0 milestone Aug 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants