-
-
Notifications
You must be signed in to change notification settings - Fork 362
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
Set cookie after login to ignore own visits #171
Set cookie after login to ignore own visits #171
Conversation
Pull Request Test Coverage Report for Build 613
💛 - Coveralls |
@electerious |
My idea was that Ackee creates a cookie for the current domain (e.g. ackee.example.com) and a request will contain this cookie (e.g. from example2.com) so Ackee can ignore it. I'm however not sure if that's possible, because of blocked third party cookies. I want to ensure that it works without |
Yes, this was my plan. I wasn't aware that using
In this case, only results from example.com will be ignored
Got it. I'll try a different approach and see if I cover both 1 and 2 above. |
Pushed another version. |
@electerious |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! Just a few small things and it's ready to be merged :)
test/resolvers/utils.js
Outdated
const mongod = new MongoMemoryServer() | ||
|
||
// Create connection to mongoose before all tests | ||
exports.before = async t => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a new function in src/utils/connect that can be used here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
src/resolvers/records.js
Outdated
return { | ||
success: true, | ||
payload: { | ||
"id": "88888888-8888-8888-8888-888888888888" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please run npm run lint
to align the code with the rest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed here and in other files in this PR
src/resolvers/records.js
Outdated
|
||
// In case of own site don't update | ||
if (req.cookies && req.cookies.ackee_login === "1") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's create a small util for this as it's used in two places. isLoggedIn(res)
=> true
/false
in src/utils/
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea. Done.
One small change: Parameter name will be req
, not res
Thanks for the great work 🙌 |
There's now an ignore-own-visits branch for both Ackee and ackee-tracker. I will do some testing on my server and on Netlify. This might take a while, but I'm sure that it will be part of the next release. The biggest outcome for me is 'MongoMemoryServer'. Haven't seen it before and it's great to add tests for all MongoDB related files. More tests are welcome (can be based on the |
Happy to help with the feature and the testing. |
Fix #100