Addresses multiple intertwining issues. #166

CoreyShupe · 2018-06-12T11:52:58Z

Fixes #71
Fixes #120
Addresses #105
Addresses #122

The reason for these all being included in this PR is they all have to do with connection and really they go hand-in-hand.

Fixes #64 Fixes #71 Fixes #120 Addresses #105 Addresses #122 The reason for these all being included in this PR is they all have to do with connection and really they go hand-in-hand. There's no way to pull specific issues from the classes without doing them together.

ianagbip1oti · 2018-06-14T01:47:18Z

src/main/java/com/github/princesslana/eriscasper/BotContext.java

  private final Observable<Event> events;

  private Routes routes;

  private RepositoryManager repositories;

-  public BotContext(Observable<Event> events, Routes routes, RepositoryManager repositories) {
+  public BotContext(
+      ErisCasper erisCasperInstance,


We should need to keep an ErisCasper instance in BotContext

ianagbip1oti · 2018-06-14T01:48:02Z

src/main/java/com/github/princesslana/eriscasper/ErisCasper.java

@@ -26,41 +30,107 @@
  private final ObjectMapper jackson = Jackson.newObjectMapper();
  private final Payloads payloads = new Payloads(jackson);

+  private final Integer[] shard;


Let's use a class provides more semantics than Integer[]

ianagbip1oti · 2018-06-14T01:48:25Z

src/main/java/com/github/princesslana/eriscasper/ErisCasper.java

        .observeOn(Schedulers.computation())
        .share();
  }

+  public Integer getShardNum() {
+    return shard == null ? -1 : shard[0];


Why return -1 when we could use Optional?

removed this alltogether, we only need the shard for identifying

ianagbip1oti · 2018-06-14T01:49:36Z

src/main/java/com/github/princesslana/eriscasper/ErisCasper.java

  public static ErisCasper create() {
-    return create(System.getenv("EC_TOKEN"));
+    String token;
+    if (System.getenv().containsKey("EC_TOKEN")) {


Using custom and varied handling of system properties/environment variables is just going to be more code we have to change when #106 gets implemented.

I'd very much prefer to use environment variables only until #106 is done.

ianagbip1oti · 2018-06-14T01:50:06Z

src/main/java/com/github/princesslana/eriscasper/ErisCasper.java

+    }
+    String shardNumRaw = System.getProperty("shard.num", null);
+    String shardTotalRaw = System.getProperty("shard.total", null);
+    if (shardNumRaw != null


This kind of validation logic is what can be shifted into a Shard class

ianagbip1oti · 2018-06-14T01:54:20Z

src/main/java/com/github/princesslana/eriscasper/gateway/commands/UpdatePresence.java

+@Value.Immutable
+public interface UpdatePresence {
+  @JsonProperty("since")
+  @Nullable


Why @Nullable instead of Optional?

Passing through null afaik means something different than not passing it through at all.

I'll look into it and see if it's optional, but from what I saw it's necessary but nullable

ianagbip1oti · 2018-06-14T01:55:00Z

...main/java/com/github/princesslana/eriscasper/gateway/commands/util/ConnectionProperties.java

@@ -0,0 +1,29 @@
+package com.github.princesslana.eriscasper.gateway.commands.util;


Why is this in its own package?

util is often a bit of a meaningless name because it can mean anything. Often it's a dumping spot because there wasn't a better name. If this is the case, wouldn't it be better off near the class that uses it?

We can put it as an inner class to Identify.

ianagbip1oti · 2018-06-14T01:56:50Z

src/main/java/com/github/princesslana/eriscasper/rx/websocket/RxWebSocket.java

        .doOnNext(e -> LOG.trace("Received: {}.", e))
        .doOnError(e -> LOG.warn("Error: {}.", e));
  }

+  private void close0() {


Why close0? Does this method name make it obvious what this method does?

it's the default closing of the socket private to the class. it's the ultimate goto when wanting to close the connection and stop accepting packets.

ianagbip1oti · 2018-06-14T01:57:23Z

src/main/java/com/github/princesslana/eriscasper/rx/websocket/RxWebSocket.java

+          socket.close(1002, "Invalid token.");
+          new ErisCasperFatalException("Failed to authenticate with discord servers.")
+              .fillInStackTrace()
+              .printStackTrace();


Don't print to stdout.

Why fillInStackTrace? Did you mean to throw this exception?

throwing it leaves a long very ambiguous looking error, seems to leave a much cleaner looking error if we directly print it, more useful to the end user imo, but I can change it.

If we can't continue, then throwing an exception is the best way to indicate this

Alright, fair enough, I'll throw it rather than printing it.

ianagbip1oti · 2018-06-14T01:58:49Z

src/main/java/com/github/princesslana/eriscasper/rx/websocket/RxWebSocket.java

            })
+        .takeUntil(event -> closed)


Referring to mutable things outside of the reactive stream is a bit of a code smell - it's kind of like the global variables of rx.

I think I can remove that, I think those are in there for no reason at this point since I added in the basic level of takeUntil

Hmm looked at it a bit more, the reactive stream needs to look outside of the reactive stream to check if the RxWebsocket is closed before it can continue. Is there a good reason we should change this? It makes the most sense for shutting down the stream so we stop sending heartbeats and such when the pipe breaks or the socket closes.

We should detect closing by seeing the Closed websocket event: https://github.com/princesslana/ErisCasper.java/blob/master/src/main/java/com/github/princesslana/eriscasper/rx/websocket/RxWebSocketEvent.java#L14

gotcha, implemented that

~ Moved gateway commands into a more Route based system ~ Changed how takeUntil receives the event and decides whether to continue ~ Changed how Shards are processed and boxed ~ Implemented configuration acceptance for sharding ~ Moved ErisCasper instance outside of the BotContext.

~ Moved gateway commands into a more Route based system ~ Changed how takeUntil receives the event and decides whether to continue ~ Changed how Shards are processed and boxed ~ Implemented configuration acceptance for sharding ~ Moved ErisCasper instance outside of the BotContext. Things left uncommitted to stand for merge?

ianagbip1oti · 2018-06-15T06:00:11Z

src/main/java/com/github/princesslana/eriscasper/api/robot/Robot.java

-   * <p>Messages are considered to be directed at the Robot if they begin with "+", the username of
-   * the bot account, or a mention of the bot account.
+   * <p>Messages are considered to be directed at the Robot if they begin with "+", the username on
+   * the bot account, or a mention on the bot account.


why are all these 'of's replaced with 'on'. I don't think it's correct in all cases.

Yeah I don't actually know when I did that.

ianagbip1oti · 2018-06-15T06:03:04Z

src/main/java/com/github/princesslana/eriscasper/gateway/commands/UpdatePresence.java

+public interface UpdatePresence extends GatewayCommand {
+  @JsonProperty("since")
+  @Nullable
+  Integer getSince();


Reading the docs to me seems like this can be absent, or present and null.

Having the option for the user to have it as null is important, Optional restricts that by leaving it out. I don't think leaving it out and having it null have the same effect.

ianagbip1oti · 2018-06-15T06:07:55Z

src/main/java/com/github/princesslana/eriscasper/gateway/commands/UpdateVoiceState.java

+    return connect(guildId, channelId, false, false);
+  }
+
+  static UpdateVoiceState connect(


I think this starts to get too much. I think the client should just use the builder themselves instead of a four parameter method. Method signatures like (Snowflake, Snowflake, boolean, boolean) are error prone also. It's easy to accidentally flip the order of parameters.

Gotcha, I'll remove the connect/disconnect methods, I thought it might be too much as well.

ianagbip1oti · 2018-06-15T06:08:51Z

src/main/java/com/github/princesslana/eriscasper/rx/websocket/RxWebSocket.java

@@ -16,6 +17,7 @@

  private static final Logger LOG = LoggerFactory.getLogger(RxWebSocket.class);

+  private volatile boolean closed = false;


This is to be removed in favor of picking up the Closed web socket event?

I need to see where else it's being used, if unused I'll remove it.

Yeah it's being removed

ianagbip1oti · 2018-06-15T06:10:36Z

src/main/java/com/github/princesslana/eriscasper/util/Shard.java

+import com.ufoscout.properlty.Properlty;
+import java.util.Optional;
+
+public class Shard implements Wrapper<Integer[]> {


I don't think implementing Wrapper here is semantically correct. I think just having a toArray method is cleaner.

Have you considered using immutables and making this a @Tuple? You could still do the checks on the values by creating our own Shard.of factory method.

Implementing Wrapper allows it to be used with jackson properly. It also is correct in the sense that it's wrapping an array of integers. If I don't use Wrapper Jackson may not like it so much. If you know how to get it to where Jackson treats it like an Integer[] like it does with Wrapper I can replace it.

Except we don't wrap an array of integers, we have two integer fields.

The Jackson magic you are looking for is @JsonValue. If you annotate the toArray method with it, then Jackson will use that to serialize.

Alright, I'll use that then.

ianagbip1oti · 2018-06-15T06:11:02Z

src/main/java/com/github/princesslana/eriscasper/util/Shard.java

+    return new Integer[] {getShardNumber(), getShardTotal()};
+  }
+
+  public int getShardNumber() {


Do we need an unwrap method along with the getters? Seems like one or the other should be redundant.

Yeah I'll probably remove the getters.

ianagbip1oti · 2018-06-15T06:12:51Z

src/main/java/com/github/princesslana/eriscasper/util/Shard.java

+  private final int shardTotal;
+
+  public Shard(int shardNumber, int shardTotal) {
+    if (shardNumber >= shardTotal || shardTotal < 1 || shardNumber < 0) {


Preconditions.checkArgument from Guava works well for this kinds of checks. It'll throw IllegalArgumentException instead of ErisCasperFatalException. I can see arguments for either.

I don't think it's necessary to mix exceptions. It's also a very primitive thing to check, I don't think it's necessary to wrap it in Preconditions.

The judgement should be made based on readability. I agree it's not a big deal with exception is throw and that the code is straight forward. Either changing or leaving is fine.

Preconditions.checkArgument(shardNumber >= 0, "shardNumber should be zero or more"); Preconditions.checkArgument(shardTotal >= 1, "shardTotal should be one or more"); Preconditions.checkState(shardNumber < shardTotal, "shardNumber should be less than shardTotal");

I might change it to preconditions then I do think that is more descriptive, tests will change with it

ianagbip1oti · 2018-06-15T23:37:13Z

src/main/java/com/github/princesslana/eriscasper/BotContext.java

  }

  public Completable doNothing() {
    return Completable.complete();
  }

+  public Completable sendGatewayCommand(GatewayCommand command) {
+    return routes.sendGatewayCommand(command);


I'm not sure this belongs on Routes. Routes is kind of HTTP specific.

But for what reason are we exposing the gateway to the user?

Gateway commands must be sent under the gateway, I dont know of a way to format them under a request since they have no actual statement. If we want to let users send commands to the gateway we need to expose it somehow. Safest way to do that is to give them access to a method which sends the command for them. I can remove Resume and Identify from GatewayCommand and it would make it a little safer in that regard.

When the user wants to do something like send a message, the fact that it's happening over HTTP is hidden from them. There is nothing in the API that refers to rest or http, they do it using the Action concept we have.

So, gateway commands can be done in the same way.

Perhaps Action is an interface, with implementations RestAction, GatewayAction.

That's kind of impractical if we want the users to know which one to use, we also need to know which one it is because each of them have special attributes. It would cause casting and checking and such if we just wanted to use Action.

ianagbip1oti · 2018-06-15T23:38:02Z

src/main/java/com/github/princesslana/eriscasper/ErisCasper.java


-  private ErisCasper(BotToken token) {
+  private ErisCasper(BotToken token, Optional<Shard> shard) {


Should we avoid this optional by having using a Shard of [0,1] as a default?

I think having no sharding is a bit safer, I dont think telling discord we are using shard [0,1] is useful and Optional makes sense and looks like what it does.

ianagbip1oti · 2018-06-24T18:19:45Z

There are still open comments on this. e.g., the exposing of 'gateway' to the user

ianagbip1oti · 2018-06-24T18:20:57Z

src/main/java/com/github/princesslana/eriscasper/gateway/Gateway.java

+                      Completable.fromAction(() -> ws.close(1002, "Invalid session."))
+                          .doOnComplete(
+                              () -> LOG.warn("Socket disconnected due to an invalid session."))
+                          .blockingAwait();


We shouldn't need to use something blocking during an observable stream.

It's blocking because we don't want anything more to even try to come through, if we do it will throw broken pipes and have issues with the websocket. If we block it then we can avoid anything even attempting to come through when it's a non-resumable invalid session.

Putting the blocking here isn't stopping other websocket events from coming through or happening.

Alright, how would we push through that command into the stream? Should we just run it outside of a completable.

I'm not really sure, this isn't something I have thought about. I'm not 100% clear on what situations lead to this happening and what action we're taking.

I'm not exactly sure what can throw a non-resumable invalid sessions in all honesty. I tried for around 3 hours to get github to throw me an invalid session without success. Invalid tokens throw an initial unauthorized message not an invalid session message so that's a bit odd. Basically right now within this codebase we're just shutting down the websocket.

Ok, so I'm not keen to merge something where we're not sure what we're meant to be doing.

Alright, maybe we should remove it then, or just throw a fatal exception when I receive an invalid session which can't be resumed. Most invalid sessions are able to be resumed and I haven't seen one where I couldn't. So maybe that's the right move? Not 100% sure.

I'd say remove anything related to #64 from this PR and address it at another time.

Sounds good to me. I think it's a more complicated issue due to being able to test and understand the circumstances that lead to the outcomes we're trying to prevent.

ianagbip1oti · 2018-06-24T18:22:52Z

src/main/java/com/github/princesslana/eriscasper/rx/websocket/RxWebSocket.java

+        socket.close(1002, "Invalid token.");
+        em.onNext(ClosingTuple.of(ws, code, reason));
+        throw new ErisCasperFatalException(
+            "Failed to authenticate with discord servers: [" + reason + "]");


The websocket class shouldn't know about the existence of discord.

Why can't the discord listeners listen for the Closing event and handle this?

Didn't even realize this was still in the codebase. I'll move it.

CoreyShupe · 2018-06-24T19:39:04Z

The gateway isn't exposed anymore? It's just mixing execute with taking more types of parameters. Once the Data gets pushed through it may be more viable to set up route type things with gateway commands and just have routes be able to handle gateway routes vs http routes. Or maybe it would be smarter to create an intervening class which handles the different and handles it the way it needs to be handled, for example if it's an http route do this, if it's a gateway route do this. So ChannelRoute and GuildRoute etc.. would implement something like HttpRoute and we can have GouldRoute and an interface Routed with a getType method to handle the special conditions of it. That might be the smartest thing to do in this situation. Although I am open to suggestions.

ianagbip1oti · 2018-06-24T19:44:47Z

The eventual goal is that all requests for data go via repositories. So, if I want to get all guild members I get a repository via the BotContext and callgetGuildMembers. Then, I shouldn't care whether it's going via the gateway, reading from something cached in memory, or via the REST API.

CoreyShupe · 2018-06-24T19:47:45Z

So really the goal is pushing all route commands / gateway commands into repositories? However the issue I see with that is most gateway commands don't return data. They aren't anything for repositories to care about, just because they are requests/commands made to the discord API to perform specific actions. Same with certain route commands. Like, prune messages, or update voice state. I'm not sure how those are at all related to repositories so I find it more into the thought of building a command API we can throw into bot context to reach all of those commands without the user knowing about the gateway or our rest API. Eventually we should never see Route<X, Y> as the user and instead be entirely concerned about Actions. Actions need to be reformed though since they don't at all accept special conditions.

ianagbip1oti · 2018-06-24T19:52:17Z

Ok, to clarify, I didn't mean all gateway commands (and http routes) should be in repositories.

Repository - where you query something about discord or the state
Action - what you use to do something

CoreyShupe · 2018-06-24T19:54:18Z

Alright, so the getGuildMembers thing you're referring to is basically telling discord I want to know more about this guild, I want to know the offline members as well, so send me GuildMemberChunk events until I have known about every single offline member to which the repository already takes care of. So it's less of a query for information which receives and more of a telling discord to send me more events. It could be seen as a repository request but the issue is it isn't returning anything or telling us anything, it's just making discord reveal more information which will be available later.

ianagbip1oti · 2018-06-24T19:56:38Z

getGuildMembers should be in a repository.

The say I see it is it should handle the GuildMemberChunk events as they come in, and present them as something like Observable<GuildMember> back to the user.

CoreyShupe · 2018-06-24T19:59:23Z

My only hesitation comes when the user doesn't understand the command. getGuildMembers returns offline members when and only when the guild has over 100 members inside of it. If it has less than 100 it will return nothing. Also, what if I call that method for the same guild multiple times? It will create 2 observables and when the events come in how will I know which observable to throw it into? The only thing I'm trying to prevent with this is user based error due to ignorance of the backend mechanics. which will prevent issues being reported when none are based off issues

ianagbip1oti · 2018-06-24T20:00:49Z

ErisCasper should take care of how getGuildMembers happens. So, ErisCasper should know whether it should get them via a gateway event, or via a REST call. Or maybe it just returns what it has cached in memory. This is why we want to put it behind the repository interface - so that the bot writer doesn't have to care about that.

CoreyShupe · 2018-06-24T20:02:23Z

Then maybe we should implement the gateway command into the initialization of the repository so the user doesn't ever have to care about it and we automagically cache every single guild member anyways. When a guild is presented we run the get guild members gateway command and it throws in chunk events so the user is always able to access every guild member.

ianagbip1oti · 2018-06-24T20:03:08Z

I'd prefer it to be lazy/on-demand

CoreyShupe · 2018-06-24T20:05:25Z

I see it more complicated than that. Just because from the gateway perspective.
The command comes in -> send events back to the user -> cache the guild members.
Should we present getGuildMembers as instead of a command just a return of the cached guild members? That might cause issues with people who want all guild members including offline ones, so maybe we should have something on the initialization defining if they want to see offline members? I'm not really sure how we should do this because the gateway command is moreso for telling discord we want more not that we have more to give to the user.

ianagbip1oti · 2018-06-24T20:11:51Z

This is why I like things as separate PRs. Because it allows more thinking and reviewing of things without blocking other changes.

From the bot writers perspective, I should call getGuildMembers on a repository. This should give me the result with the principle of least surprise (i.e., it should give me all members of the guild).

How we implement that, is a separate discussion, and yeah, there are issues to work through like you raised (e.g., if we cache, how do we know the cache is valid?).

But, Observable is a great abstraction for presenting the GuildMembers back to the bot writer. They write code that operators on the Observable, that is called when the GuildMembers become available - it doesn't expect them to all be there immediately.

CoreyShupe · 2018-06-24T20:14:26Z

Alright fair enough. In all honesty, I may strip gateway commands entirely from here considering we're handling them through Data and representing them here in multiple different ways. I don't think gateway commands can be handled in the current state of the codebase. I think we should first reformat the Actions API and the repositories to handle information better and have a more viable state. I think right now we're moreso in the stage of setting up the skeletal framework and the framework for gateway commands is being done in -Data now.

Stripped Gateway commands due to a rethought of how we're handling them. Moved invalid token handling to the gateway. Stripped invalid session handling due to uncertainty.

ianagbip1oti · 2018-07-01T04:35:58Z

src/main/java/com/github/princesslana/eriscasper/ErisCasper.java


-  private ErisCasper(BotToken token) {
-    this.token = token;
+  private Gateway gateway;


the gateway is initialized later, not when created

Is gateway used out side the getEvents method?

I'll probably remove the field altogether and add it with the action rewrite

ianagbip1oti · 2018-07-01T04:37:24Z

src/main/java/com/github/princesslana/eriscasper/rx/websocket/RxWebSocket.java

  public Completable send(String text) {
    return Completable.fromAction(() -> ws.send(text))
        .doOnComplete(() -> LOG.trace("Sent: {}.", text));
  }

  private static class Listener extends WebSocketListener {
    private final ObservableEmitter<RxWebSocketEvent> em;
+    private final RxWebSocket socket;


This is unused?

I can remove this, yeah, leftover

ianagbip1oti · 2018-07-01T04:37:47Z

src/main/java/com/github/princesslana/eriscasper/rx/websocket/RxWebSocket.java

        .doOnNext(e -> LOG.trace("Received: {}.", e))
        .doOnError(e -> LOG.warn("Error: {}.", e));
  }

+  public void close(int code) {


Is this used?

yeah I think I can remove that now

CoreyShupe · 2018-07-02T11:25:36Z

@ianagbip1oti this needs to be commented on.
The scope of this now addresses how we're doing gateway commands.
Implements a sharding technique (which I think will be replaced by 0.6.0 data classes) but is useful for a placeholder until we move the API object into Data. (fixed stage 1 and addresses the entire issue)
Fixes bad tokens.

ianagbip1oti · 2018-07-02T15:29:15Z

Which is the best path forward to 0.6.0 data? Making a PR to upgrade to data 0.6.0 and merging that before updating this, or merging this and then modifying what's here for data 0.6.0?

CoreyShupe · 2018-07-02T15:33:56Z

I think the most logical is merging this then modifying. If we update to 0.6.0 then merge the PRs might get messy. Once 0.6.0 is updated we need to make everything conform anyways. There's going to be quite a bit in the update PR just because of how much changed between 0.5.0 -> 0.6.0. Not much is changing within this PR besides the sharding pieces afaik. This also kind of keeps things consistent with time and commit consistency for everyone. Neater.

CoreyShupe added the enhancement New feature or request label Jun 12, 2018

CoreyShupe added this to the 0.4.0 milestone Jun 12, 2018

CoreyShupe assigned ianagbip1oti Jun 12, 2018

CoreyShupe requested a review from ianagbip1oti June 12, 2018 11:52

CoreyShupe added 5 commits June 12, 2018 07:55

Removed useless @nullables, assumed null by convention.

611e144

Better sharding assertion, 0counting num, 1counting total.

56006e1

Removed termination exiting.

1cb99c9

Moved closed on test to the end of the method.

5e01f47

Fixed json issue with Identify.

48ea8cf

ianagbip1oti requested changes Jun 14, 2018

View reviewed changes

CoreyShupe added 8 commits June 15, 2018 00:36

Refactored sharding from config.

e28614a

Testing tests fix.

37187e8

Test fixes.

4ba0fe2

Formatting + shard exception throwing when 1 value present.

62e34ea

Fixed sharding assertion and added tests for config creation.

513d66e

Styling.

cc7076e

ianagbip1oti reviewed Jun 15, 2018

View reviewed changes

Reverting Robot.

2d90faa

ianagbip1oti reviewed Jun 15, 2018

View reviewed changes

General style fixes.

c8a2cbb

ianagbip1oti reviewed Jun 15, 2018

View reviewed changes

ianagbip1oti reviewed Jun 24, 2018

View reviewed changes

Merge branch 'master' into master

9107b15

CoreyShupe added 2 commits June 24, 2018 16:50

Stripped certain pieces and moved others.

159b44f

Stripped Gateway commands due to a rethought of how we're handling them. Moved invalid token handling to the gateway. Stripped invalid session handling due to uncertainty.

Small spotless fix.

ea62d00

ianagbip1oti reviewed Jul 1, 2018

View reviewed changes

CoreyShupe and others added 2 commits July 1, 2018 10:03

Merge branch 'master' into master

f4f76ab

Unused item reduction.

b0114e9

Merge branch 'master' into master

0d4f0bd

ianagbip1oti approved these changes Jul 2, 2018

View reviewed changes

ianagbip1oti merged commit d511ee5 into princesslana:master Jul 2, 2018

		@@ -0,0 +1,29 @@
		package com.github.princesslana.eriscasper.gateway.commands.util;

		@@ -16,6 +17,7 @@

		private static final Logger LOG = LoggerFactory.getLogger(RxWebSocket.class);

		private volatile boolean closed = false;


		private ErisCasper(BotToken token) {
		private ErisCasper(BotToken token, Optional<Shard> shard) {

Addresses multiple intertwining issues. #166

Addresses multiple intertwining issues. #166

Conversation

CoreyShupe commented Jun 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianagbip1oti commented Jun 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CoreyShupe commented Jun 24, 2018

ianagbip1oti commented Jun 24, 2018

CoreyShupe commented Jun 24, 2018

ianagbip1oti commented Jun 24, 2018

CoreyShupe commented Jun 24, 2018

ianagbip1oti commented Jun 24, 2018 • edited Loading

CoreyShupe commented Jun 24, 2018

ianagbip1oti commented Jun 24, 2018

CoreyShupe commented Jun 24, 2018

ianagbip1oti commented Jun 24, 2018

CoreyShupe commented Jun 24, 2018

ianagbip1oti commented Jun 24, 2018

CoreyShupe commented Jun 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CoreyShupe commented Jun 12, 2018 •

edited

Loading

ianagbip1oti commented Jun 24, 2018 •

edited

Loading