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

Add Gov-Client-Public-IP and Gov-Client-Public-IP-Timestamp header to HMRC headers #131

Closed
wants to merge 3 commits into from

Conversation

RiyaJohn
Copy link
Contributor

@RiyaJohn RiyaJohn commented Oct 7, 2021

Additional Gov-Client-Public-IP and Gov-Client-Public-IP-Timestamp headers

Description of what's changing

Adds Gov-Client-Public-IP and Gov-Client-Public-IP-Timestamp headers to HMRC headers
Fixes #125 and #126
...

What else might be impacted?

...

Which issue does this PR relate to?

#125 and #126
...

Checklist

[ *] Tests are written and maintain or improve code coverage
[ ] I've tested this in my application using yarn link (if applicable)
[ *] You consent to and are confident this change can be released with no regression

@RiyaJohn RiyaJohn changed the title Add Gov-Client-Public-IP-Timestamp header to HMRC headers Add Gov-Client-Public-IP and Gov-Client-Public-IP-Timestamp header to HMRC headers Oct 7, 2021
@@ -49,6 +49,7 @@
},
"dependencies": {
"@babel/preset-env": "^7.10.4",
"axios": "^0.22.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of this additional dependency, can you please use fetch API to make the API call https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API/Using_Fetch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure I can change that

@skodamarthi
Copy link
Contributor

@RiyaJohn I and @reubenae had a discussion about this and we felt relying on a 3rd party website to fetch the IP address is not good. I am currently looking into other options and I will keep you posted on this. Also, feel free to suggest if you know of any better ways to fetch the public IP address. For now, please hold off making any code changes, till we finalise the approach. Thank you!

@RiyaJohn
Copy link
Contributor Author

@RiyaJohn I and @reubenae had a discussion about this and we felt relying on a 3rd party website to fetch the IP address is not good. I am currently looking into other options and I will keep you posted on this. Also, feel free to suggest if you know of any better ways to fetch the public IP address. For now, please hold off making any code changes, till we finalise the approach. Thank you!

Thanks for the update, sure will try

@skodamarthi
Copy link
Contributor

@RiyaJohn After further discussion, we have decided to close this issue for now as introducing this header in the library could cause issues to the existing customers, and adding this header using 3rd party APIs felt more of an unnecessary overhead than a benefit. Apologies for the confusion regrading this! We just created some more good-first-issues to export the headers individually. Please take a look at them if you are interested to contribute to the other issues. Thank you!

This pull request was closed.
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.

Add Gov-Client-Public-IP header to HMRC headers
2 participants