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

Universal analytics #259

Merged
merged 17 commits into from May 2, 2014
Merged

Universal analytics #259

merged 17 commits into from May 2, 2014

Conversation

@fofr
Copy link
Contributor

@fofr fofr commented Apr 17, 2014

  • Show a hits summary and errors, archives and redirects across all sites
  • Highlight which URLs are performing the worst
  • Help identify and fix mapping problems

screen shot 2014-04-15 at 16 49 08
screen shot 2014-04-15 at 16 48 37
screen shot 2014-04-15 at 16 48 17

@jamiecobbett
Copy link
Contributor

@jamiecobbett jamiecobbett commented Apr 22, 2014

Nice 👍 We should definitely show this at Show and Tell.

@@ -1,5 +1,6 @@
class HitsController < ApplicationController
before_filter :find_site, :set_period
skip_before_filter :find_site, :only => [:universal_summary, :universal_category]

This comment has been minimized.

@jamiecobbett

jamiecobbett Apr 22, 2014
Contributor

I think this would be clearer as:

before_filter :find_site, except: [:universal_summary, :universal_category]
before_filter :set_period

This comment has been minimized.

@fofr

fofr Apr 22, 2014
Author Contributor

Yes, that's much clearer. Done.

@jamiecobbett
Copy link
Contributor

@jamiecobbett jamiecobbett commented Apr 22, 2014

Is there a reason not to link to edit the mapping?

@fofr
Copy link
Contributor Author

@fofr fofr commented Apr 22, 2014

Most users won't have access to edit the majority of what's in the top 10 lists. I also like to think of it as a tool to prompt further investigation, rather than quick fixes.

<%= link_to hits_path, class: 'list-group-item' do %>
<h4 class="list-group-item-heading">Universal analytics</h4>
<p class="list-group-item-text text-muted">
See how <%= @site.default_host.hostname %> compares with the traffic of all other sites transitioning to GOV.UK.

This comment has been minimized.

@jennyd

jennyd Apr 23, 2014
Contributor

This link suggests to me that it leads to a specific comparison of this site to all others. In fact many sites won't appear at all in the first few pages of the global hits list, which could be confusing to users.

This comment has been minimized.

@fofr

fofr Apr 29, 2014
Author Contributor

This has now been removed.

@jennyd
Copy link
Contributor

@jennyd jennyd commented Apr 23, 2014

I've just noticed that the time period dropdown doesn't work without javascript for me (this also affects the site hits pages, since it's the same partial). I know it's not modified in this PR, but we should make sure that it's possible to select a different time period without javascript.

@fofr
Copy link
Contributor Author

@fofr fofr commented Apr 23, 2014

@jennyd We have a very old pivotal story to fix that: #60234868

@jennyd
Copy link
Contributor

@jennyd jennyd commented Apr 23, 2014

@fofr Great, let's do it!

<%= link_to hit.host.site.organisation.title, site_path(hit.host.site), class: 'link-muted' %> &middot;
<%= link_to 'Analytics', summary_site_hits_path(hit.host.site), class: 'link-muted' %>
<% if hit.mapping %>
<% if [hit.http_status, hit.mapping.http_status] == ['410', '301'] %>

This comment has been minimized.

@jennyd

jennyd Apr 23, 2014
Contributor

🔎 There are 2 spaces before the %> at the end of this line 🔍

<strong class="breakable path">
<%= link_to hit.default_url, hit.default_url %>
<div class="hit-organisation">
<%= link_to hit.host.site.organisation.title, site_path(hit.host.site), class: 'link-muted' %> &middot;

This comment has been minimized.

@jennyd

jennyd Apr 23, 2014
Contributor

Linking to the site dashboard with the organisation title feels a bit odd to me, and in general we don't conflate organisations with sites. Do we need a link to the site dashboard at all here, or would just the site analytics link be enough? If the aim is to make it easier for users to identify hits on sites which they are responsible for, we could eg highlight rows for sites which their org can edit or include the add/edit mapping button for them instead.

Having these links here makes the 'was archived, now redirecting' text less obvious, which seems especially important to show clearly on this page, because we're implicitly comparing users on how quickly they fix their highest-traffic mappings. Can we visually distinguish that text from the links a bit more, so it's easier to see when scanning down the table?

This comment has been minimized.

@fofr

fofr Apr 23, 2014
Author Contributor

All good points. I think I'll address the main concern by leaving the organisation name in whilst removing the link.

The aim of the feature is to quickly find the biggest problems across all sites and use the data to notify others who are responsible for those mappings. The feature is built from a "GDS Analyst" perspective.

Have you got a screenshot where the "was archived/now redirecting" isn't scannable?

This comment has been minimized.

This comment has been minimized.

@fofr

fofr Apr 23, 2014
Author Contributor

That screenshot is showing some cached CSS, the second line is slightly indented to make the whole thing more scannable.

This comment has been minimized.

@jennyd

jennyd Apr 23, 2014
Contributor

Ah yes - that helps a bit, but I still find it difficult to distinguish the 'was archived, now redirecting' from the 'Environment Agency · Analytics' within that line:

screen shot 2014-04-23 at 16 56 00

This comment has been minimized.

@fofr

fofr Apr 28, 2014
Author Contributor

Looking at this again, I think that the "was archived, …" bit is the least important piece of information on this line, and that it doesn't deserve to be distinguished.

This comment has been minimized.

@fofr

fofr Apr 29, 2014
Author Contributor

It's now clearer that the link is to the site.

@fofr
Copy link
Contributor Author

@fofr fofr commented Apr 28, 2014

I have some concerns about how HMRC analytics may break these stats pages.

@fofr
Copy link
Contributor Author

@fofr fofr commented Apr 29, 2014

After discussing with Dave, Universal Analytics are now only linked to for admins (though everyone can still access). This should mitigate the risk from HMRC analytics.

Paul Hayes added 12 commits Apr 14, 2014
* Show top errors, archives and redirects across all sites
* See top errors, archives and redirects across all sites
* Defaults to last 30 days
* From the main navigation bar, removing the redundant Organisations
link
* From the site dashboard, beneath site analytics
* Include a link to a hit’s site using organisation name
* Remove actions column
* Add a link to the analytics for that site
* Currently an inconsistency between visited and normal links, one
stays grey, one goes blue
* Now they both go a darker gray
* When /a comes from two different hosts we need to show them as
separate entries in the global hits view
* Include host_id in the grouping
* Try and factor out some of the magic numbers in hits assertions,
define what we expect when setting the hits data
* Base feature on hits_summary
* A hit model can now provide a default URL (based on the default host)
* link_to_global_hit was poorly named
@jamiecobbett
Copy link
Contributor

@jamiecobbett jamiecobbett commented May 2, 2014

This needs rebasing to be mergeable again. 🐎

Paul Hayes added 4 commits Apr 17, 2014
* Re-use existing hits partials where sensible
* Avoid “global”, use “universal”
After discussions with Dave Mann about who this feature is for, to
avoid confusion and to mitigate the risk that HMRC will have on this
feature, the navigation and site links have been removed.
Paul Hayes
* Don’t link to site from organisation, because that’s confusing
* Use the text “Site” to link to site, with hostname as a title
@@ -1,7 +1,8 @@
class HitsController < ApplicationController
include BackgroundBulkAddMessageControllerMixin

before_filter :find_site, :set_period
before_filter :find_site, :except => [:universal_summary, :universal_category]
before_filter :set_period
before_filter :set_background_bulk_add_status_message

This comment has been minimized.

@jamiecobbett

jamiecobbett May 2, 2014
Contributor

I happened to have an unprocessed job, so this triggered an error on the universal hits page. I think it should sit next to and have the same exceptions as :find_site.

This comment has been minimized.

@fofr

fofr May 2, 2014
Author Contributor

Fixed in 4001f1b

jamiecobbett added a commit that referenced this pull request May 2, 2014
@jamiecobbett jamiecobbett merged commit 0fd5a57 into master May 2, 2014
1 check passed
1 check passed
default "Build #527 succeeded on Jenkins"
Details
@jamiecobbett jamiecobbett deleted the global-hits branch May 2, 2014
@jamiecobbett
Copy link
Contributor

@jamiecobbett jamiecobbett commented May 2, 2014

We should demo this at the next Show and Tell 🐴

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.