Tests, Refactoring, and More

[jpitlor]

Hello, everyone!

This is a pretty huge PR. There are tons of changes. I'm opening it now so people can have a chance to look at the code. A lot of it I don't plan on changing before a formal review, so if you want to make comments, please do. The earlier we catch bugs, the better!

Be warned, I have not actually ran the tests to make sure they work. I'm still finishing writing up the mock data, so that part will be changing, but everything else shouldn't.

Later in this PR, I will make sure to write:

• A changelog
• A migration guide

Hopefully, after we merge this, maintaining the server and adding features will be easier, and hopefully it's easier to read so newcomers aren't quite as daunted

Side note: I'm definitely going against the typical goal of branches in that this is a "super branch" doing a lot of things at once. It's impossible to get fixed now, but I suggest not doing this moving forward

[hughesjeff]

Please update the route to be /v2/ since this is a breaking change.

[hughesjeff]

Can we also get a route to get all room names?

[hughesjeff]

Can we either update this to 8.x latest, or downgrade to the latest LTS?

[jpitlor]

I bumped to 8.4, but I would not feel comfortable always pulling the latest. We should stay at one version and bump as needed so nothing breaks out of the blue. We also cannot go below 7.9 because we'd lose async/await support

[jpitlor]

RE node version: yes, i can bump it up
RE rooms route: yes, that's trivial
RE v2 - which routes? i don't think i changed any output or input of any routes unless i'm forgetting something. i tried to keep the interface identical

[hughesjeff]

The room names have changed versus the old version since we're getting them dynamically now. We used to have them hard-coded, and since they have changed names, the app will crash if you try to use it on the new server. I updated the app to work against this and Android O on https://github.com/Purdue-ACM-SIGAPP/PurdueLaundry-android/tree/server-update-v2 branch.

[hughesjeff]

I'd like to update the app to pull the rooms from the server, so once we get the routes done, I can work on implementing that.

[jpitlor]

At this point we have a green build. I have ignored 8 tests which test getAllMachines and getMachinesAt. These are a little bit more difficult to test since there are more things to mock. The latter I will be able to unignore with minimal effort soon, but the former we should refactor first. I created #43 for it

[jpitlor]

@hughesjeff I'm revisiting code changes now that the build is green. why would the names of rooms have changed? the old information never changed, just new rooms added

[jpitlor]

Also @hughesjeff can you redo your code review? github is getting mad that the commit hashes are missing since i rebased from your codeowners commit

[hughesjeff]

The room names used to be hard coded with their URLs. Now that we're getting them dynamically, they've changed. So it went from cary to Cary Quad West Laundry in the JSON response.

[jpitlor]

D'oh! I completely forgot about those keys. Oops! Ill get the old route back for the v1 and change that to v2

[jpitlor]

@hughesjeff I added that route in, so if there's nothing else you wanted me to add, i'll mark the requested changes as made

[jpitlor]

Alo @vidia @vieck we're probably merging this relatively soonish, so review the code if you want

[hughesjeff]

So I wasn't certain how we wanted to handle this. We can discuss during the meeting tomorrow, but I'm uncertain if we want to keep the v1 and v2 instances separate and tie them together with nginx, or put them together like this.

[jpitlor]

Sounds like a good thing to discuss tomorrow

…

On Mon, Aug 21, 2017, 10:44 AM Jeff Hughes ***@***.***> wrote: So I wasn't certain how we wanted to handle this. We can discuss during the meeting tomorrow, but I'm uncertain if we want to keep the v1 and v2 instances separate and tie them together with nginx, or put them together like this. — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#38 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEoTU_oAFIp70LYpVxLobZvV8-gkk_sxks5saZffgaJpZM4N5SZ7> .