-
Notifications
You must be signed in to change notification settings - Fork 26
Conversation
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.
I cannot see any test in this and therefore I won't merge. This PR is a perfect exercise how to do testing, as we have to stub out external requests. I would "record" the response of an external request and store the JSON in a file, check it into the repository, and reload the response from that file to avoid external requests. From ruby land I love https://github.com/vcr/vcr or https://github.com/oesmith/puffing-billy for the sledge-hammer solution.
Also I don't quite understand when a user gets geo-located and what is the expected result in the database. This is where a cucumber test would come in handy, where you can hard-code the expected result into the cucumber scenario.
src/middleware/userMiddleware.js
Outdated
} | ||
} | ||
|
||
const fetch = url => { |
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.
@appinteractive why do we have to implement our own fetch?
src/middleware/userMiddleware.js
Outdated
parent = ctx | ||
}) | ||
} | ||
// delete all current locations from user |
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.
Is there such a thing like a database transaction in neo4j for atomic operations?
|
||
export default { | ||
Mutation: { | ||
CreateUser: async (resolve, root, args, context, info) => { |
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.
We create a user in our tests. @appinteractive can you check if we have external requests on our build server now? This is bad practice as it makes the build unstable and it produces unnecessary traffic.
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.
Yes it does but I don't know how to quick fix that currently.
@appinteractive always run the docker-compose up workflow at least once.
@appinteractive Running ``` docker-compose -f ../Nitro-Backend/docker-compose.yml -f ../Nitro-Backend/docker-compose.travis.yml up ``` from the Nitro-Web folder gives me an error when running seed data. The configuration in this commit fixes it on my machine. I guess that will fix the frontend build, too.
@appinteractive, it's super strange but apparently the JWT has to be enclosed in quotes whereas the MAPBOX_TOKEN must not be enclosed. This commit fixes almost all tests running in the docker containers on my machine. The remaining test case is: ``` About me and and location I set my location to "Mecklenburg-Vorpommern" (example #2) ``` @appinteractive when I try to repeat the cypress test on my machine I get no results for "Mecklenburg-Vorpommern" only cities and states. can you check if regions really work?
Ready for merge @roschaefer |
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.
@appinteractive what happens if the frontend gets deployed before the image of the backend was updated?
@roschaefer you can’t login then |
As a user
I’d like to add my location
So others can find me by city, region or country and or see from where I am.
As a user
I’d like to add a short description about me
So others can get a better picture who I am.
Todos:
Related PR`s
Human-Connection/Nitro-Web#9