Skip to content
This repository has been archived by the owner on Feb 26, 2023. It is now read-only.

TODO List before deployment #1

Open
9 of 11 tasks
RoiEXLab opened this issue Aug 18, 2017 · 6 comments
Open
9 of 11 tasks

TODO List before deployment #1

RoiEXLab opened this issue Aug 18, 2017 · 6 comments
Assignees
Milestone

Comments

@RoiEXLab
Copy link
Member

RoiEXLab commented Aug 18, 2017

Mandatory:

  • Better Error Messages (Currently a lot error messages is just passed through). We need to seperate "internal issues" which should just be printed to the log from "user-related issues" which the user should be informed about
  • Send dice emails after rolling/Check if users are registered
  • Prettier Emails in general (Maybe use a templating engine?)
  • HTML sites allowing users to register their emails (Templating engine?)
  • Verify user input is actually emails, currently there are no checks which verify the emails are actual single emails and not a list which would be processed by our sendEmail command and sent to several users, also we need to limit registration requests, so our server can't be used to spam innocent people (also hurts our reputation), also don't send a registration email if the user is already registered, and return an error instead for consinstency.

Optional:

  • Improve the error logging system internally, currently a lot of logging is done "by hand", we don't have something like an error builder, which returns the result if everything works out, and returns an error status with message otherwhise. This includes automated checking of params while only passing maps of predicates and error messages. The error-handler.js file was my "might finish sometime" attempt to adress this, haven't had the time.
  • Have user interface and UI in parallel: This means, without the /api prefix an html page based on the JSON should be displayed instead of the raw JSON
  • Have an automated setup command. Currently a config.json needs to be created manually, we should provide a setup command or something similar
  • TESTS. (Also travis integration). It might work out on our test enviroment, but nothing is better thana good amount of tests. (Haven't ever used test on a node environment, so any insights would be helpful)
  • Restructuring. If everything is done, we might should consider on improving the folder structure instead of storing everything in the src folder.
  • Cryptographic dice rolls? Might be overkill, but it might be "more random" if we didn't rely on Math.random() to generate our numbers, but on crypto.randomBytes() instead.
@ssoloff
Copy link
Member

ssoloff commented Aug 18, 2017

@RoiEXLab Not sure where you want comments on this stuff, so I'm just leaving them here for now. Please let me know if you'd like them moved.

Have POST and GET requests work in parallel, there are cases in which GET requests are required, but they clutter the URL quite a lot, so if we were able to use both, that would be a huge plus.

Yes, the URLs are quite cluttered. 😄 They are also very brittle because of the parameter positions. And I'm not sure the positions are intuitive. If you're going to keep GETs around, I would consider just using a query string, as it makes the parameters more obvious. This API is more RPC than REST, so the URL doesn't have to look RESTful.

I suspect the GET for the roll endpoint is not idempotent because it will probably generate a different response given the same request. That's Not Good. Also, I haven't actually looked at the code, but if this request has any side effects (e.g. updating a database, sending emails, etc.), it's not safe, and that's really Not Good for a GET. You might want to just use POST for this specific endpoint. I suspect the verify endpoint is idempotent and safe, so it can probably remain a GET.

@RoiEXLab
Copy link
Member Author

I suspect the GET for the roll endpoint is not idempotent because it will probably generate a different response given the same request. That's Not Good.

@ssoloff I thought about that, and you're right, but for now GET request make testing so much easier, going to change that though.
Also GET request are mandatory for verification to generate email-clickable links

@RoiEXLab
Copy link
Member Author

@ssoloff Also the idea is to support urlencoded POST params as well as JSON encoded params, the only difference being the url params being unescaped and mapped to real "objects" While the json params are expected to be ints, floats, strings, arrays etc. (exception Date string)

@RoiEXLab
Copy link
Member Author

@ssoloff I put a lot of work in this repository today, which overhauled the complete code base.
I did almost no testing, but with a correct config file and some luck it could work out as expected.
So the only missing requirement would be to have a nice user interface where users can register themselves.
Although we could technically implement a client into TripleA that registers users automatically when they first use the dice server.

I'd like to hear your opinion on this.

@ssoloff
Copy link
Member

ssoloff commented Nov 19, 2017

I did almost no testing, but with a correct config file and some luck it could work out as expected.

Famous last words. 😄

So the only missing requirement would be to have a nice user interface where users can register themselves.

Just to be clear... you're suggesting having no web UI like MARTI does for user registration? Just a web service that could be used for registration by the TripleA client?

If so, there's probably a way to work it into the dice server configuration screen on the Play by PBEM/PBF menu. Possibly have a separate Register... button that brings up a dialog that prompts for the username, sends the HTTP request, and parses the response. However, the user would still have to return to their email client to click the verification link

@RoiEXLab
Copy link
Member Author

@ssoloff

Just to be clear... you're suggesting having no web UI like MARTI does for user registration? Just a web service that could be used for registration by the TripleA client?

Not exactly suggesting, just wanted to bring up this idea, but yes.

@RoiEXLab RoiEXLab mentioned this issue Nov 25, 2018
@RoiEXLab RoiEXLab added this to the 1.0.0 milestone Dec 18, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants