dbudwin / robohome-web Goto Github PK

There is a strong use of the doesUserOwnDevice(...) function. While it works well, it's kind of bulky to repeat over and over. It would be nice to find another way handle this check, maybe as middleware or a function in a base class.

Also, I don't think I like the name of the function. It doesn't read super smoothly. Maybe simplify to just ownsDevice(...)

Example code in question:

$doesUserOwnDevice = $this->currentUser()->doesUserOwnDevice($id);
 
if (!$doesUserOwnDevice) {
   $request->session()->flash(FlashMessageLevels::DANGER, 'Error updating device!');
 
   return redirect()->route('devices');
}

Having unit tests for controllers is critical to making sure the web application routes and handles user web requests as expected. For example, testing to make sure a user can't delete a device that belongs to someone else. Or testing that if you try to view a protected page and are not logged in, the user is redirected to the log in page. If any other additional utilities are needed (like a mocking framework) I'm open to adding them too in their own individual PR.

I think doing a PR per controller class is appropriate and I'll gladly support adding the Hacktoberfest label to any of them!

PHPUnit with the DBUnit add-on would be suitable testing frameworks.

Fat Free also has a testing framework which could be good for testing routes: https://fatfreeframework.com/unit-testing

A critical part of this project is to be able to control RF devices using a NodeMCU as the microcontroller. There currently isn't support for 433 MHz RF transmitters in the form of a driver/library written in Lua.

Example transmitter: https://www.amazon.com/SMAKN%C2%AE-433Mhz-Transmitter-Receiver-Arduino/dp/B00M2CUALS

The library used for MQTT already supports TLS, we just need to take advantage of it: https://github.com/McFizh/libMQTT

Specifically, it looks like the function is called setCryptoProtocol() in that library.

This should be possible using the API, the tricky part is the authentication using OAuth.

I do not own a Google Home, so I will have to depend on the community to help with this.

Much like how this application supports Amazon Echo, a similar set of capabilities should exist to support Google Home. This will likely have to be many PRs.

• Support for logging in via another method may be needed. Currently only support logging in using Amazon credentials. The logic being that if you own an Echo you have to have an Amazon account. By doing this, users didn't have to sign up with a new set of credentials to use RoboHome. Options to solve this could be:
  • Add support for logging in with Google accounts
  • Take advantage of Laravel's support for user accounts and registrations
  • Use another technology/tool to handle accounts and registrations
• Users should be able to pass commands to Google Home

As of PR #89, it's possible to do UI browser tests. Currently, it only tests the index page. The device page will also benefit from testing but this is non-trivial given the log in wall and using a 3rd party to authenticate with (Login With Amazon). Ever since #121 it has become feasible to test the devices page using Dusk's loginAs($user) function.

Expected Behavior

The RoboHome logo should be add in the upper left-hand corner of all pages in the website. The logo should appear to the left of the "RoboHome" text. The logos can be found here: https://github.com/dbudwin/RoboHome-Web/tree/master/public/images

Further, the logo should be hyperlinked to return to the main page at /devices. It should also still display nicely when viewed on a mobile device.

Current Behavior

Currently, just the text "RoboHome" appears.

Most of them are in place, but it should be added and corrected everywhere to fit this format:

public funcion myFunction(): void
public funcion myFunction(): array
public funcion myFunction(): MyType

Change id to deviceId for turnOn and turnOff API methods. This will make it consistent with the info API method.

When a user gets a 404 error by going to a URL that doesn't exist, they should be present with a 404 error page that informs them that the URL doesn't exist and provide a button to go back and a way to get to the home page. This 404 error page should match the look and feel of the rest of the website.

Expected Behavior

Going to a non-existent URL will present the user with a helpful 404 error page that tells them it was a 404 error and provides a link to go back to the previous page, and there should also be a link to go to the home page as well. The 404 error page should match the styling of the rest of the website. A creative/fun 404 page is also acceptable.

Current Behavior

Currently when going to non-existent URL, you get the default Laravel error message that says: "Sorry, the page you are looking for could not be found." There is no good information about what when wrong, or how to go back (besides clicking the back button).

Possible Solution

Create a 404 error page by using Laravel's supported means for handling HTTP errors: https://laravel.com/docs/5.5/errors#custom-http-error-pages

Anything you'd want to know about the API can be found here: https://developer.amazon.com/public/solutions/alexa/alexa-skills-kit/overviews/understanding-the-smart-home-skill-api

This seems like it should be pretty straightforward, it mainly just adds some new fields to the existing API. Most of the changes will occur in API/DevicesController.

For example, this will allow us to take advantage of the new field displayCategories to make sure devices in RoboHome are grouped correctly in the Alexa app.

Expected Behavior

When a user wants to delete a device, they click the "Delete" button and the action is completed. It would be preferable to have a confirmation that asks if they are sure that they want to delete the device. It should also work well when viewed on a mobile device.

Current Behavior

The device is immediately deleted when the user clicks the "Delete" button.

Possible Solution

It would be acceptable to have a small modal/dialog box appear that says something like "Are you sure you want to delete [insert device name]?" with a "Yes" and a "No" button. The default selected behavior should be the "No" action so a user doesn't excitedly delete a device. This also means that pressing the "Enter" button on the keyboard will not have an effect. It will also be necessary to add a Dusk test case or two to verify this behavior.

Expected Behavior

Using Bootstrap, a static fixed footer appears at the bottom of each page with a basic copyright notice like "© 2018 by RoboHome." The word "RoboHome" should be a URL to this repository. It should also display correctly on a mobile device. A simple Laravel Dusk test case or two to verify that the footer on the page is present will be useful.

Current Behavior

Currently there is no footer.

I couldn't get the app running by using the StartDockerAndRunTests.sh file provided.

Expected Behavior

The web.sh command in StartDockerAndRunTests.sh file should have returned the web container name that was generated during docker-compose.

Current Behavior

The commands in the StartDockerAndRunTests.sh fail when it encounters lines with web.sh, because the container name for the laravel app during docker-compose up does not match the name that is generated by the web.sh file.
generated name: robohome-web_web_1_ae1757cab9c1
expected name: robohomeweb_web_1
I'm using the latest version of docker-compose btw. Could you please tell me if i'm missing anything here?

Possible Solution

One workaround I figured was to name the container in the compose yml file with
container_name: robohomeweb_web_1

Before the port to Laravel last year, it used to be possible to use Docker to spin up this project. This was quite handy for testing purposes and took away some of the necessary effort to configure the project for people new to the project.

The old docker-compose.yml file can be used as inspiration for getting this functionality working again.

It should support at the very least:

• PHP 7.1
• MySQL
• phpMyAdmin

When running docker-compose up it should be possible to get to a local instance of the RoboHome website.

The crossed out section of the README will need to be un-crossed out, and updated too.

Expected Behavior

Devices with the exact same name (as entered by the user) should not be allowed for a user. You should not be able to create a device with a name matching another device nor should you be able to edit an existing device and attempt to change the name to that of another existing device. Different users can have devices of the same name.

Current Behavior

There is no validation when entering devices names to see if it conflicts with other device name for a user.

Possible Solution

This should be enforced on the server side and a helpful message should be given to the user to let them know of the conflicting device names.

There will need to be a page for user settings that will be useful for current and future functionality. This page should allow the user to enter and edit MQTT information (channel, server, user, password, port) which will be shared with the ESP-8266 to reduce the number of settings that have to go into the sketch. It should also display known user information like email address, Amazon user ID, etc.

The on/off buttons for controlling RF devices are currently placeholders. Add in calls to CloudMQTT API to send the signals so they can be picked up my other devices like the NodeMCU. The MQTTPublisher file should be turned into a F3 controller class.

Should be useful as a favicon and larger. Also should contain the source file for creating the logo.

Being able to tag devices (i.e. Downstairs Lights) will make controlling multiple devices simultaneously feasible. Currently, if there are two lights downstairs, I have to ask for each light to be turned on individually. This should be controlled via a single command like: "Alexa, turn on [tag name]."

It'll be great to have unit tests set up for the models in this project to ensure they persist and retrieve data to the MySQL database correctly. I've already set up a testing framework per issue #9 which supports testing databases. If any other additional utilities are needed (like a mocking framework) I'm open to adding them too in their own individual PR. I recently included the MySQL schema in PR #13 for anyone that may need it.

I think doing a PR per model class is appropriate and I'll gladly support adding the Hacktoberfest label to any of them!

The .travis.yml will need to be modified to include HHVM and any breaking changes will need to be resolved as well. If for whatever reason it's not possible to support HHVM with a reasonable amount of effort, the reason why should be detailed for future reference.

Currently, the only form is the "Add Device" form. It should implement validation to make sure it's filled out before submission.

Currently, the "Edit" button on a device doesn't do anything. I think it should popup the modal used for adding devices with the information pre-populated and editable.

There are several solutions to this:

• The most quick and dirty one is to use resolve() to create an instance of the DevicesController and call it's devices method
• A better solution is to extract the devices method into either a Trait or to the base Controller class so that it could be reused across classes
• Another one is to implemented a route middleware, but the middleware's method would have the same type hint issue as it will have to return either a Redirect or a Closure

It's not preferable to pass basic integers that correspond to the table's auto-increment PK to control which devices to get/edit/delete. Using a unique identifier like a UUID would be preferable and more secure. A new column will need to be added to the tables to support this.

When using Faker for creating tests, we should enforce consistency for types.

i.e. sometimes we use randomDigit() for on code values and sometimes we use randomNumber(). Which one is chosen isn't particularly important (I think I prefer randomNumber()). Maybe worth looking into aliasing types so that certain values (user ID, on code, off code, pulse length, etc.) will always consistently return the same type.

Expected Behavior

The info method in RFDeviceInformation should return the device's pulse length in addition to the code needed to turn a device on or off. The sketch should then be modified to use this value.

Current Behavior

Currently, the pulse length is hardcoded into the microcontroller sketch.

Possible Solution

Add the pulse length to the JSON response from the info method in RFDeviceInformation.

Context

Because of this, the pulse length value is useless. If it's anything other than 184 (the hardcoded value), it won't work.

Currently, there is no validation that a user can perform an action on a certain device. In theory, a user could delete someone else's device just by knowing their ID. Validating a user owns a certain device will fix this. I was thinking this could be done by creating a controller that inherits Controller and implements the beforeRoute() F3 method. Or a method in Controller as part of Controller's beforeRoute() method could handle validation. Along with #2, this should make access and control of devices much more secure.