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

Limit Persistent Anonymous Data Collection Prompt To Control Panel #13948

Closed
wants to merge 3 commits into from

Conversation

zero-24
Copy link
Contributor

@zero-24 zero-24 commented Feb 6, 2017

Pull Request for Issue #13943 .

Summary of Changes

Testing Instructions

Install Joomla
Log In
Do not make a selection for Anonymous Data from the prompt
Navigate to some other pages

Expected result after the patch

The message is only showed in the control panel.

Actual result

Message is unnecessarily persistent all screens.
It's annoying and has a negative impact UX on all screens outside the control panel.

Documentation Changes Required

None

Additional comments ([copied from original issue by @cpfeifer])

I support the option to allow people to submit this data and it is very useful to our process, but there is no reason for it to be persistent across all screens unless our intent is to force users to make a decision, which it should never be.

This option is presented to our users as a way to help improve Joomla, but the persistent message essentially destroys UX everywhere outside the control panel by popping up and pushing all features down the screen considerably until they make choice. The choice they make may be out of frustration or a desire to make the message go away, and this potentially skews the data regarding the choices users are making.

If it's limited to being persistent on the dashboard, they'll see it when they login, which I still don't like but it's better than the way it is now. I think the presentation of this option should be completed revisited in J4, but I feel limiting it to the control panel is a fair compromise for now. I hope we can do something about this sooner than later.

User Feedback:
"Given that I have clients using the back end, if they see the messages it confuses them, and scares them."

@wilsonge
Copy link
Contributor

wilsonge commented Feb 6, 2017

-1 for me. We had a long discussion about this at the time of implementation. This was the solution rather than forcing users to accept. I'm strongly against going even further backwards on this

@mbabker
Copy link
Contributor

mbabker commented Feb 6, 2017

I still don't get why the project feels like the stats collection notice should be in your face everywhere while the 2.5 EOL message and now our PHP EOL message get locked to the control panel only; those are more urgent things to be communicating to users.

@ghost
Copy link

ghost commented Feb 6, 2017

We are throwing it in their face and forcing them to make a choice, that's the issue. Having this on the dashboard is one thing, having it on the article editing screen and every other screen is a whole other thing. It's too aggressive.

@ghost
Copy link

ghost commented Feb 6, 2017

I have tested this item ✅ successfully on fc717c7


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

@ghost
Copy link

ghost commented Feb 6, 2017

Here is a video of my test and I think it perfectly demonstrates the issue. I started with a fresh install and had to install and verify the patch tester without clicking the prompt. It's a huge distraction having the message pop up every single time on every single screen when you're trying to do something. Every time my mouse stalls out, that's my brain trying to process everything on the screen getting pushed down.

I can't believe for a second that any Joomla user is okay with this the way it is. Let's fix this now.

anon-msg

@@ -94,6 +94,12 @@ public function onAfterInitialise()
// Load plugin language files only when needed (ex: they are not needed in site client).
$this->loadLanguage();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code seems fine. But would it not be best to put it above $this->loadLanguage();? No need to load the language if it will potentially return

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, @zero-24 can you move it after the if statement.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That break the translation and result into a not translated message:

image

As the option is not set to com_cpanel on that screen. if i add it manually it works but not without.
administrator/index.php?option=com_cpanel

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, didn't think about that.

@C-Lodder
Copy link
Member

C-Lodder commented Feb 6, 2017

Agree with Babker, so I'd personally suggest moving this to a post installation message (as that's pretty much what it is)

@zero-24
Copy link
Contributor Author

zero-24 commented Feb 6, 2017

@C-Lodder i have tryed this and this resulted into a not translated message :(

I personally dont care. The reason it is not a postinstall is because we asuming that sending is allowed after a few time (i cant remember the actual time we wait) This is also the reason we need to force the user to take a chooice. ;)

@Bakual
Copy link
Contributor

Bakual commented Feb 6, 2017

On the other hand, just press one of the buttons and it will never show up again.
I always do that directly after installation since it annoys me to much (which it does by design)

@ghost
Copy link

ghost commented Feb 6, 2017

Put yourself in the shoes of a person who has never used Joomla before. You install for the first time, this message is the first you see when you login, then it follows you everywhere until you choose. What message are we sending to new users?

We need to think about that more. We're all used to this, but from an onboarding perspective it's a nightmare. This does not deliver a pleasant first user experience, and first impressions count for a lot. That's where my head is at.

@ghost
Copy link

ghost commented Feb 7, 2017

Show on Control Panel is enough. After next Update the Message come again, User knows more about Reasons why its helping to click "Always send".

@ghost
Copy link

ghost commented Feb 22, 2017

Decision of Production Department?

@Sieger66
Copy link
Contributor

Sieger66 commented Mar 3, 2017

I have tested this item ✅ successfully on fc717c7


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

@zero-24
Copy link
Contributor Author

zero-24 commented Mar 4, 2017

Moving to Needs Review on request by @yvesh


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

@mbabker
Copy link
Contributor

mbabker commented Mar 16, 2017

RTC

Please note per a discussion with Tobias that apparently moving the load language call after the cpanel check causes the messages to not translate. Let's move forward with this as is and we can address that issue separate.


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Mar 16, 2017
@zero-24
Copy link
Contributor Author

zero-24 commented Mar 16, 2017

This PR is wrong.. As with this PR we just include the js in com_cpanel and if we do so there come no updates to the stats server (you need to be on the cpanel that the code is triggerd). I'm working on a fix.

@zero-24
Copy link
Contributor Author

zero-24 commented Mar 16, 2017

I need some more input :(

The plugin is trigger twice.. the first time with com_cpanel as option and the seccond time with com_ajax.
This means if we add any check like above we add the js only on com_cpanel to the actual site, wich is wrong i think as this way the stats sending is not triggerd anywhere else than com_cpanel.

Or do we get the decision that it is ok that we are only sending the stats if we are on the cpanel?

@zero-24
Copy link
Contributor Author

zero-24 commented Mar 16, 2017

If we have that decision i can also fix the languages part by moving that code to the onAjaxSendStats method.

@zero-24
Copy link
Contributor Author

zero-24 commented Mar 16, 2017

In order to fix this: #14615 we can move that the code to onGetIcons event (wich is only triggerd in com_cpanel too)..

@mbabker
Copy link
Contributor

mbabker commented Mar 16, 2017

IIRC the design was so that the collection could happen regardless of who is logged into the site or where (same way the update notification email plugin triggers independently of an admin logging into the site). So we shouldn't limit that part to someone logging into the site and hitting the control panel.

Use the plugin's isAjaxRequest method if you need to do something conditional based on full page request or AJAX.

@zero-24
Copy link
Contributor Author

zero-24 commented Mar 16, 2017

IIRC the design was so that the collection could happen regardless of who is logged into the site or where (same way the update notification email plugin triggers independently of an admin logging into the site). So we shouldn't limit that part to someone logging into the site and hitting the control panel.

That is also broken by this line: https://github.com/joomla/joomla-cms/blob/staging/plugins/system/stats/stats.php#L79

@zero-24 zero-24 removed the RTC This Pull Request is Ready To Commit label Mar 16, 2017
@mbabker
Copy link
Contributor

mbabker commented Mar 16, 2017

I give up. Ping me if the server side breaks 🤣

@zero-24
Copy link
Contributor Author

zero-24 commented Mar 16, 2017

I give up. Ping me if the server side breaks

hehe 😄 Thanks anyway. Your reply means we need to find a fix for that.

@yvesh
Copy link
Member

yvesh commented Mar 17, 2017

You have to add the original option from which com_ajax is called in the JavaScript :-) Than you can decide there if you render or not..

@zero-24 zero-24 closed this Jun 11, 2017
@zero-24 zero-24 deleted the zero-24-patch-2 branch June 11, 2017 21:45
@laoneo
Copy link
Member

laoneo commented Jun 12, 2017

Why did you close it?

@zero-24
Copy link
Contributor Author

zero-24 commented Jun 12, 2017

It did never worked correctly. If you have a solution you can do a new PR :)

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.

None yet

10 participants