tdmalone / slackemon Goto Github PK

eg. https://travis-ci.org/tdmalone/slackemon/jobs/242321511 - expand the coveralls submission towards the bottom to see that it can't read tests/logs/clover.xml.

I've had a quick look into this and can't see why - pretty sure I didn't change anything. Will try to look into further when I have some time.

v0.0.38 added support for AWS being used for the data store/data cache. This means that Slackémon can now finally be used on a ephemeral filesystem service like Heroku.

However, there are issues inherent in this:

• AWS is an eventually consistent filesystem, which theoretically could cause some player and battle data to be lost, overridden, or to temporarily step back in time.
• Slackémon was originally written around using the filesystem as a data store, so it is quite liberal in its opening of and writing to files. While this doesn't seem to cause too much latency when the calls are coming from nearby (eg. Heroku is also on AWS), it can quickly add up AWS costs.
• There is no concept of file locking built in. This is already an issue that exists even when Slackémon is run with its own local data store: a player trying to perform certain actions simultaneously, across different Slack messages or button actions, could potentially find they lose some data or even worse, corrupt their player JSON file if it is read by a local system while it is being written to.

Slackémon currently has separate settings for an image cache and a data cache, they can both be set to either 'local' or 'aws' independently. However, we're utilising the data cache as both a data cache and a data store.

We likely could solve the above issues by:

1. When 'aws' is set as the data cache, also using a secondary local cache to reduce reads from AWS for the life of the current filesystem.
2. Implementing an additional option for the data store, allowing either 'local' or an alternative store such as Redis or Postgres.
   • Slackémon currently uses JSON files to store player and battle data (one per player and one per battle) so it would be ideal, to avoid having to rewrite alot, to stick somewhat to this method if possible. However, perhaps each individual player's Pokémon could be separated into a separate file to prevent single JSON strings from ever getting too big.
   • The method chosen would need to be easy to set up and support. Both Redis and Postgres are simple on Heroku; we would just need to look into Docker options too.
3. Implementing some sort of file lock for the data store, whether 'local' or the above alternative option is in use.

In addition, while in-memory caching is already in use for most data cache reads (eg. Pokémon & species data, move data, item data) and data store reads (player data and battle data), we could also probably reduce writes by ensuring that there's one save to a player file per request (at the moment there are sometimes two especially due to the slackemon_add_xp() function being called while something else requiring a save is also happening, such as catching a Pokémon).

When using items or teaching moves, Pokemon names in message menus no longer come back in alphabetical order (they used to).

There might possibly be a bug in the sorting rules.

define( 'CRON_TOKEN', 'XXXXXXXXXXXXXXXXXXXX' );

Could this variable be autogenerated randomly?

Maybe on mobile view, just use text?

Given that the Postgres data store is now active, we also need a way to prune stale spawn, battle and move data - otherwise the database is just going to fill up and reach limits (eg. 10k row limit on Heroku free).

All of the content in spawns, battles_active, battles_complete, battles_invites and moves can be cleared after 24 hours (even earlier probably, but this should be safe), and this could be done by cron.

Currently the data cache only works locally, which is not feasible for ephemeral filesystems.

While the image cache functions work whether set on either 'local' or 'aws', the data cache is not yet set up to work on AWS. It will need read and write operations set up, unlike the image cache which only writes and which is accessible publicly.

To finish the battle you have to make a 'pointless' move.

We need to detect fainting of a Pokemon after its own move, not only after an opposing move.

Soon after I put this project on Github, and after @Naramsim's first pull requests, I slightly changed how I was branching. I noticed that @Naramsim then started setting his pull requests to go into my new tim branch - which is essentially my 'develop' branch.

To make things clearer, I'm thinking of adopting A successful Git branching model, minus the release branch flow for now (probably because we're in alpha, we generally just make releases when there's a feature or two that's ready, so that could happen straight from the develop branch). Version bumps would then happen directly after a merge from develop into master.

This would mean that each 'feature' I work on will have its own branch, which is not too different from how I have been developing tests and data-store. However, rather than merging into tim, I would instead merge into develop, which is also where @Naramsim and others could make their pull requests into.

Sound alright? @Naramsim?

This is caused by the move to PokeCSS graphics, which stores as eg. nidoran-male and nidoranm, but we're looking for nidoran-m (and same for the -f sprites).

Best way to fix is probably to duplicate all these sprites in the media repo to also be available at the -m / -f suffixes.

Transferring a Pokemon holding an item should either result in the item being returned to your bag (probably the preferred option), or the transfer failing/not being possible.

To assist in this, the held item icon could possibly also be shown next to a Pokemon on the bulk transfer list.

Following the move to using environment variables, there are some optional settings that can no longer be easily adjusted by users.

These probably need to be moved to config.php and amalgamated into the get_env() structure.

Should load the emojis

Wurmple's evolution chain is reported incorrectly due to a bug in PokeAPI.

Since PokeAPI is not being actively maintained at the moment, we may need to find a way to 'override' data retrieved from the API in this case... to correct the bug with this bug's evolution chain.

Re: PokeAPI/pokeapi#294

I knew this was coming at some stage but didn't realise it would be so soon.

Slackémon cannot run without Pokeapi.

I have forked it just in case (although Paul isn't going to remove it), but we'll need to look into setting up an instance of it before 1st July. Probably on Heroku? However, Pokeapi gets a lot of use and I won't be able to support everyone who wants to use it, so the instance will probably only be set up for use by Slackémon.

In mobile mode, when viewing Pokemon in your collection, special and speed stats are hidden for space reasons. These often are important (well, the special stats anyway, given that speed isn't implemented in battles yet), and probably still need to be visible.

In addition, it might be nice to see the special and speed stats on the spawn message as well (both for mobile and desktop views).

Slackémon was originally part of another framework but since moving out has both retained the original structure of that framework and added additional integrations such as CI. Configuration for additional integrations pollutes the root folder, especially when there's some source code mixed in there too. In addition, from the original framework, some of these files do not much else but call counterparts in the slackemon/ folder... which in turn end up calling functions in _functions/pokemon, some of which use functions directly in the _functions folder.

Some of the functions are also prefixed pokedex_ rather than slackemon_, which reflect that that they were originally written for a Pokédex feature part of the same framework Slackémon came from.

Whew! So basically, the structure needs rethinking so it's clear and less confusing. At the same time, we should clear the root folder of any source code, and try to put some of the directly run code into functions where they can be properly tested.

Some functions - in particular some spawn, catch and battle routines - have quite high cyclomatic and NPath complexity. This makes it difficult to write tests for them, and of course makes development of some new features difficult and increases the possibilities of introducing bugs.

Some current functions need to be re-thought in order to resolve this.

The first versions of Slackémon - which ran inside an existing proprietary Slack framework in our office - had a feature that locked staff out from playing it while they had a toggl timer running (this was how I avoided the risk of being asked not to continue making Slackémon available :) ).

That's not in the current version, because it depended on features available in that framework's environment.

To move my office over to the new current version - that is, the one here on GitHub! - I need to re-implement that, but obviously now in a hookable way.

I'm thinking perhaps an option to set a webhook URL that will be called periodically, sending across Slack user IDs of active users and requesting their 'status'. That webhook URL could then respond with a flag for those users that should be 'locked' out. This would allow me to respond in a custom manner from our office when Slackémon users have a toggl timer running - and will hopefully allow anyone else to integrate their own custom logic in (eg. lock people out during business hours, etc.)

Because Slackemon has been newly separated from the Slack framework it was in previously ('Slashies'), it still retains some links such as old constant names that need to be updated.

A lot of these have been updated now but a few may still remain, and currently the first user onboarding on a new deployment does not go through smoothly.

This is probably the most important step to take next to make it easier for new people to jump on board.

Users need to be prevented from running key commands (eg. /slackemon maybe-spawn) - unless APP_ENV is development.

Need to ensure that all possible errors, warnings and notices that PHP reports are effectively set to go somewhere the administrator will see them.

• On Heroku, Papertrail works well for this, but we need to ensure the config is correct so every error goes there by default.
• On Docker, we need to ensure we're setting up the right settings and have somewhere to send the data. Maybe just to the error_log file will be fine? (although if not mounting the local file system over /var/www/html its possible it won't ever be seen)

Then we can also remove the ini_set()'s from init.php.

When message menus are used for selecting a Pokemon (eg. to teach it a move, or use an item on it), on iPhone when you search there are always no results. It works fine on Windows and web, and on the Android app as well.

This may be a Slack iPhone app bug, but there's a chance something was implemented incorrectly in our use of the Slack API.

The AWS SDK is currently included directly in the repository (via the .phar file).

It should be moved to Composer instead, and tested that image caching still works ok.

Code coverage recently went up from 0% to 4%... yay! But that's still pretty low.

Every file in _functions needs reviewing and unit tests written for at least a portion of functions. Some functions will need to wait until their complexity is reduced, but there's a lot of tests that can be written now.

Slackemon has been built with only adhering manually to code standards, and also across slightly different environments.

Some older files use tabs for indentations whereas newer ones use spaces. And while the code is generally clean, there's no enforcing of standards and there certainly are parts of the code that should be cleaner.

Need to build an appropriate set of PHPCS rules, clean everything up, and then possibly make it part of the Travis build process.

The sample icon suggested in the README for users creating their Slackemon Slack apps is 128 x 128 and no longer accepted by Slack:

Icons must be squares between 512px by 512px and 2000px by 2000px, please!

Need to find an additional icon to use as an app icon (we still need the existing one for emoji use, as larger icons aren't accepted for that).