{"action":"created","issue":{"url":"https://api.github.com/repos/kizniche/Mycodo/issues/122","repository_url":"https://api.github.com/repos/kizniche/Mycodo","labels_url":"https://api.github.com/repos/kizniche/Mycodo/issues/122/labels{/name}","comments_url":"https://api.github.com/repos/kizniche/Mycodo/issues/122/comments","events_url":"https://api.github.com/repos/kizniche/Mycodo/issues/122/events","html_url":"https://github.com/kizniche/Mycodo/issues/122","id":181466403,"number":122,"title":"Model design improvements","user":{"login":"etiology","id":5456095,"avatar_url":"https://avatars.githubusercontent.com/u/5456095?v=3","gravatar_id":"","url":"https://api.github.com/users/etiology","html_url":"https://github.com/etiology","followers_url":"https://api.github.com/users/etiology/followers","following_url":"https://api.github.com/users/etiology/following{/other_user}","gists_url":"https://api.github.com/users/etiology/gists{/gist_id}","starred_url":"https://api.github.com/users/etiology/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/etiology/subscriptions","organizations_url":"https://api.github.com/users/etiology/orgs","repos_url":"https://api.github.com/users/etiology/repos","events_url":"https://api.github.com/users/etiology/events{/privacy}","received_events_url":"https://api.github.com/users/etiology/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":0,"created_at":"2016-10-06T16:39:12Z","updated_at":"2016-10-06T23:45:52Z","closed_at":null,"body":"There are a few areas where the models can be improved to prevent bugs.  Keep in mind that this product works, and it works really well.  That's an awesome accomplishment.  I just wanted to list them as I see them so that there is some visibility.  We can fix these as we go along with updates:\r\n\r\nThis is kind of related to #114 for DB improvements but this can be done now instead of waiting.\r\n\r\n1. **Use of ```str``` types** instead of a table relationship or Enum:\r\nThe use of a string as a type property can create issues.  The string is case sensitive and one can make typos.  This should be changed to something more static.\r\n - The string property for type (and [order](https://github.com/kizniche/Mycodo/blob/a958d5c14b7beb7371aec0980ee4dc91efb6957c/mycodo/databases/mycodo_db/models.py#L48)) is an example of this model object knowing too much about other things.  Why does it need to know about what types there are?  Instead we should make a ```MethodTypes``` object and create a foreign key relationship to it.  This [encapsulates](https://en.wikipedia.org/wiki/Encapsulation_(computer_programming)) the property and protects the ```Method``` class from knowing to much about types.  Adding new types is easy since we just add it to the ```method_type``` table, and nothing has to change in the  methods table.  \r\n - Also look at the other models for areas that fall into this category.  Sensor has a few of these properties that should be tables, and I'm certain that there are other models that could be subject to this.\r\n\r\n2. **Foreign Keys constraints should be used to create relationships** instead of performing a software evaluation later.  Look at [```RelayConditional```'s use of the ```if_relay_id``` property](https://github.com/kizniche/Mycodo/blob/a958d5c14b7beb7371aec0980ee4dc91efb6957c/mycodo/databases/mycodo_db/models.py#L139).  In software later a comparison is made [to evaluate the a potential relationship](https://github.com/kizniche/Mycodo/blob/a958d5c14b7beb7371aec0980ee4dc91efb6957c/mycodo/controller_relay.py#L262).  This should be a constraint instead of an evaluation.  The constraint makes the behavior of the relay and its controller much more robust in a changing environment.  \r\n\r\n3. **All models need to define \\_\\_repr\\_\\_**.  This is considered part of best practices.  It really helps later on for debugging and instance outputs.\r\n\r\n4. **Standard Model Behavior** . All models should have certain things that are common behaviors.  Similar to the issue with string base PKs in #115. We should move all primary keys to integers and if we need a unique constraint for printing to the user then we should make a property that is set to unique and defaults to something like a generated UUID.\r\n\r\n5. The ```user_restriction``` [in the user model](https://github.com/kizniche/Mycodo/blob/master/mycodo/databases/users_db/models.py#L40) should either link to a ```role``` table or, if it is just used for defining admin vs normal user then it should be changed to a boolean property called ```is_admin```"},"comment":{"url":"https://api.github.com/repos/kizniche/Mycodo/issues/comments/252118997","html_url":"https://github.com/kizniche/Mycodo/issues/122#issuecomment-252118997","issue_url":"https://api.github.com/repos/kizniche/Mycodo/issues/122","id":252118997,"user":{"login":"kizniche","id":838427,"avatar_url":"https://avatars.githubusercontent.com/u/838427?v=3","gravatar_id":"","url":"https://api.github.com/users/kizniche","html_url":"https://github.com/kizniche","followers_url":"https://api.github.com/users/kizniche/followers","following_url":"https://api.github.com/users/kizniche/following{/other_user}","gists_url":"https://api.github.com/users/kizniche/gists{/gist_id}","starred_url":"https://api.github.com/users/kizniche/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/kizniche/subscriptions","organizations_url":"https://api.github.com/users/kizniche/orgs","repos_url":"https://api.github.com/users/kizniche/repos","events_url":"https://api.github.com/users/kizniche/events{/privacy}","received_events_url":"https://api.github.com/users/kizniche/received_events","type":"User","site_admin":false},"created_at":"2016-10-06T23:45:52Z","updated_at":"2016-10-06T23:45:52Z","body":"All great suggestions. I'm not sure what you mean with a couple statements, but I trust your judgement. I'll have to brush up on the terminology you're using to fully understand what you're talking about ;)"}}
{"actor":{"display_login":"kizniche"}}