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

Cross domain tracking for HMRC renew tax credits #810

Merged
merged 3 commits into from Jun 5, 2015
Merged

Conversation

@fofr
Copy link
Contributor

@fofr fofr commented Jun 2, 2015

HMRC will be promoting https://www.gov.uk/renewtaxcredits which sends people to their service at
https://www.tax.service.gov.uk/tax-credits-service/start. Attempt to follow users from the paid-for promotions through to the completion of the service.

https://www.pivotaltracker.com/story/show/95971198

HMRC will be promoting https://www.gov.uk/renewtaxcredits
which sends people to their service at
https://www.tax.service.gov.uk/tax-credits-service/start.

Attempt to follow users from the paid-for promotions through to the
completion of the service.

https://www.pivotaltracker.com/n/projects/1261204
'accelerated-possession-eviction' => 'UA-37377084-12',
'renewtaxcredits' => 'UA-43414424-1', # HMRC, https://www.pivotaltracker.com/n/projects/1261204
}.each do |slug, account_code|
if @publication.slug == slug %>

This comment has been minimized.

@edds

edds Jun 3, 2015
Contributor

This is starting to turn into quite a lot of logic in this view. Is it worth moving some of this logic into either the presenter or a helper? So the view could just call something like if @publication.ga_account_code or if account_code = ga_account_code_for_publication(@publication).

This comment has been minimized.

@fofr

fofr Jun 3, 2015
Author Contributor

I agree, I've added a helper in 6bc6afb

@bradwright
Copy link
Contributor

@bradwright bradwright commented Jun 3, 2015

Should we add a test that the helper does what we expect?

@fofr fofr force-pushed the cross-domain-tax branch from 6bc6afb to d940889 Jun 3, 2015
@fofr
Copy link
Contributor Author

@fofr fofr commented Jun 3, 2015

@bradleywright I've added a unit test

{
'register-to-vote' => 'UA-23066786-5',
'accelerated-possession-eviction' => 'UA-37377084-12',
'renewtaxcredits' => 'UA-43414424-1', # HMRC, https://www.pivotaltracker.com/n/projects/1261204

This comment has been minimized.

@edds

edds Jun 3, 2015
Contributor

Can we lose this link to the pivotal board. If there is information in there which should be attached to this code it would be better to put that in the commit message or the comment. This code will probably outlive the pivotal board.

This comment has been minimized.

@fofr

fofr Jun 3, 2015
Author Contributor

All the content contained within the story is already in the commit.

This comment has been minimized.

@fofr

fofr Jun 3, 2015
Author Contributor

While the link is available, it is useful. Link rot for the history and context of our backlog is the problem that ought to be fixed. Not having a permanent link back to context/discussion for any given story makes excavation more difficult, even if commits themselves are descriptive and useful.

This comment has been minimized.

@edds

edds Jun 3, 2015
Contributor

In which case can this be updated to a link to the relevant story rather than a link to a board then.

This comment has been minimized.

@fofr

fofr Jun 3, 2015
Author Contributor

Fixed

This comment has been minimized.

@evilstreak

evilstreak Jun 3, 2015
Contributor

Having the link in the pull request is great, and Github makes it easy to go from a commit to a pull request. For example, 141d945 shows that this commit is part of #810.

Having the link in the commit is fine, it means we're not reliant on the Github history, though that's probably at least as persistent as our Pivotal Tracker project will be. Getting hold of that information is pretty straightforward using git blame.

Having the link as a comment in the code seems pointless, and suggests that to properly understand what's done here the reader should follow through that link.

I'm strongly in favour of dropping the link from the code.

This comment has been minimized.

@fofr

fofr Jun 3, 2015
Author Contributor

I disagree, but have removed the link in favour of getting this merged.

@bradwright
Copy link
Contributor

@bradwright bradwright commented Jun 3, 2015

👍

@fofr fofr force-pushed the cross-domain-tax branch from 710d3d7 to 20a11a1 Jun 3, 2015
fofr added a commit that referenced this pull request Jun 5, 2015
Cross domain tracking for HMRC renew tax credits
@fofr fofr merged commit dd1f184 into master Jun 5, 2015
1 check passed
1 check passed
default "Build #768 succeeded on Jenkins"
Details
@fofr fofr deleted the cross-domain-tax branch Jun 5, 2015
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

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