-
Notifications
You must be signed in to change notification settings - Fork 18
Conversation
README.md
Outdated
[![Coverage Status](https://coveralls.io/repos/github/frederike-ramin/gamification/badge.svg?branch=coveralls)](https://coveralls.io/github/frederike-ramin/gamification?branch=coveralls) | ||
# gamification | ||
[![Build Status](https://travis-ci.com/schul-cloud/gamification.svg?branch=master)](https://travis-ci.com/frederike-ramin/gamification) | ||
[![Coverage Status](https://coveralls.io/repos/github/schul-cloud/gamification/badge.svg?branch=coveralls)](https://coveralls.io/github/frederike-ramin/gamification?branch=coveralls) |
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.
The link still points to the old repository
README.md
Outdated
## Testing | ||
|
||
Simply run `npm test` and all your tests in the `test/` directory will be run. | ||
Run `npm test` to run all tests. Use `npm coverage` to run tests and generate coverage. |
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.
Maybe change this to: "Run npm test
to run all linters and tests. Use npm coverage
to also generate coverage."
I find that makes it a bit clearer, especially since tests won't even run if the linter fails.
c87b77d
to
630fcc1
Compare
I've written a bunch of docs now. I'd welcome a review :) |
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! Thank you! I made some comments, most of which are nitpicking, but also some questions on parts I wasn't sure about.
README.md
Outdated
## About | ||
This project provides a microservice to manage gamification for your | ||
application. It does not provide any graphics or pre-defined content, but | ||
only a backend REST api. At its core, the service listens to events sent |
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.
API is spelled in all caps
README.md
Outdated
- the user reached 10 XP | ||
- the user achieved the FooAchievement | ||
- a FooEvent is received | ||
|
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.
Maybe mention that a combination of rules can be required for an achievement. (e.g. user has 10 XP and FooAchievement)
README.md
Outdated
Each user has a level. The user starts at level 1 and can never go below that. | ||
The level is strictly tied to the user's main xp "XP". The required xp for each | ||
level can be configured to either be linearly increasing, exponentially | ||
increasing, or completely custom-defined. The level is not stored anywhere but |
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.
Double space
README.md
Outdated
are granted, these are persisted to MongoDB. It is planned to also send an event | ||
back to RabbitMQ when an achievement or xp are granted. | ||
|
||
The gamification service also provides a (mostly) readonly REST api, which can |
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.
read-only instead of readonly
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.
Huh. Must've used too much TypeScript lately 💃
README.md
Outdated
Define the different xp-pools there are within your application. | ||
The "XP" xp-pool is always defined. | ||
|
||
Note: This section is not yet used anywhere within the gamification service. |
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.
What do you mean 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.
I've rewritten the paragraph to be more clear (I hope):
Note: The xp-pool names defined here are not currently validated by the
configuration parser, i.e. not specifying the available xp-pools or specifying
additional xp-pools does not raise an error currently. It is, however, planned
to validate the configuration more thoroughly, which would then raise an error
if you use an xp-pool not defined 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.
Yes, that makes it clear to me. 👍
README.md
Outdated
You can send events manually in the *Exchanges* section. Select the exchange and then publish your message at *Publish message*. Don't forget to insert the *routing key*. | ||
Define when to increase a user's level based on their XP. The level is | ||
always purely based on the user's current XP. Each user, regardless of this | ||
configuration, always starts at level 0. That means that the first |
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.
Shouldn't this be Level 1?
README.md
Outdated
|
||
1. Achievements: The achievements service handles achievement data and stores | ||
granted achievements in the `achivements` collection. Each row consists of the | ||
`user_id`, the `name` (achievement name), `amount` (how often the user has |
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.
Shouldn't this be current_amount
and total_amount
? In other sections, you already talk about maxAwardedTotal
, so the new values from your PR should also be included here.
README.md
Outdated
|
||
Simply run `npm test` and all your tests in the `test/` directory will be run. | ||
This project provides two `docker-compose` files, one meant for development and | ||
one meant for production. The |
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.
The rest of this sentence is missing.
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.
Really good job, thanks a lot, @cmfcmf!
If you find any issues such as typos, feel free to directly push to this branch.
This fixes most of #25 and depends on #62 and #57.