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

Iqss/6725 analytics bug #6726

Merged

Conversation

qqmyers
Copy link
Member

@qqmyers qqmyers commented Mar 4, 2020

What this PR does / why we need it: Fixes a bug in the analytics-code.html example file in the docs, which breaks button-level tracking for Google analytics.

Which issue(s) this PR closes:

Closes #6725

Special notes for your reviewer:

Suggestions on how to test this:
The issue shows a javascript console error that should go away when the fix is applied.

Does this PR introduce a user interface change?:

Is there a release notes update needed for this change?:
Up to you - I created one. To fix the bug, installations have to reinstall the analytics-code.html file or modify their existing one. It does look to me like this has been broken since ~v.417 - hard to tell if it was a bug/typo in the code or if a UI change broke things at some point. (Pretty sure it worked to start but I can't say when the UI change might have happened.) That said, it only affects installs that have deployed a custom analytics file though.

Additional documentation:

@coveralls
Copy link

coveralls commented Mar 4, 2020

Coverage Status

Coverage remained the same at 19.444% when pulling 3597435 on QualitativeDataRepository:IQSS/6725-analytics-bug into c4dd4a5 on IQSS:develop.

@mheppler mheppler self-assigned this Mar 5, 2020
@mheppler
Copy link
Contributor

mheppler commented Mar 5, 2020

@qqmyers looking at the latest and greatest version of this example code in the develop and master branches, I do not see a typo/missing character.

https://github.com/IQSS/dataverse/blob/develop/doc/sphinx-guides/source/_static/installation/files/var/www/dataverse/branding/analytics-code.html

Is it possible this may only be an issue on your fork?

@qqmyers
Copy link
Member Author

qqmyers commented Mar 5, 2020

The issue is in line 120 - the '>' character there only finds direct children which is not the case. It should be removed.

@mheppler
Copy link
Contributor

mheppler commented Mar 5, 2020

D'oh. Apologies. I was seeing your changes backwards, while looking at our analytics code and wonder how we already had the direct child selector... 🤦‍♂.

But, while I was in there, I did do a little more digging into the dataset pg source code for the files table, and looking at those td.col-file-metadata table columns. I think that rather than relying on the selector to through that entire block of code, we could keep the original direct child selector and use a different container.

<div class="file-metadata-block">
    <a href="/file.xhtml?persistentId=doi:10.7910/DVN/XXXXXX/XXXXXX&amp;version=2.0">
        Sample.txt
    </a>
...

I haven't tested it, but I believe we could use div.file-metadata-block > a to achieve the same selection of the file id/DOI in the dataset page.

@qqmyers
Copy link
Member Author

qqmyers commented Mar 5, 2020

@mheppler - that looks fine too. If you change it, the release notes should change to match.

@mheppler
Copy link
Contributor

mheppler commented Mar 6, 2020

@qqmyers PR QualitativeDataRepository#39 submitted. Release notes did not specify the change, so it did not need revising.

Changed selector in analytics exmaple to be more precise [ref IQSS#6725]
Copy link
Contributor

@mheppler mheppler left a comment

Choose a reason for hiding this comment

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

👍

@mheppler mheppler removed their assignment Mar 6, 2020
@kcondon kcondon assigned kcondon and mheppler and unassigned kcondon Mar 6, 2020
@kcondon kcondon assigned kcondon and unassigned mheppler Mar 6, 2020
@kcondon kcondon merged commit 7ed6519 into IQSS:develop Mar 6, 2020
@djbrooke djbrooke added this to the 4.20 milestone Mar 9, 2020
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.

analytics tracking broken for download button
5 participants