Closed pedanticantic closed 5 years ago
Fixes and Changes of Functionality
I tried very hard not to change any functionality. However, I had to in a couple of places, plus I noticed a couple of bugs, which I fixed.
Change of Functionality
Fixes
Oh, when I first started, I did an npm install and a bower install, and these things changed hundreds and hundreds of lines in package-lock.json
In theory they should be checked, but there are so many changes, I don't know if it would be practical!
General Comments: Changes look great. I like the TODO comments you've added - after these changes have landed we should go through them and determine what we want to do in those cases.
I see what you mean about the image upload. I think your solution makes sense because of how we need to structure and upload the images. We'll likely need to add some more error checks around the images in the future but for now this looks good.
We should separate the admin dashboard files and the customer facing files for organizational reasons. We should also try and get some tests in place as well, but again that can be done in the future
Thank you both for looking at this PR. I know it was huge and everything.
I agree about going through the TODOs. They are (if I remember correctly) a combination of potential problems I found and ideas for improving what I'd done.
I think generally it needs to cope with errors a bit better. It's not a criticism of course - when one builds an application, one is focused on getting the functionality working and puts error handling on the back burner.
Yes, separating the "user" and "admin" code is a good idea - definitely the controllers and views. In theory, the models and repositories can stay the same, but I'm open to suggestions.
Yes, I never got round to trying to write any tests. As I've said elsewhere, I've never written tests in JavaScript (I've done it a lot in PHP), so it will be a chance for me to learn!
I basically tried to do a couple of things:
So the new structure has a services folder which contains classes that interact with the data/file stores. The "factory" class receives the firebase objects, and instantiates & provides a DataRepository object (talks to the data) and a StorageRepository (talks to files). There is a PersistService that does processing that might involve both repositories. These are the only things that interact with date/file stores directly. There is also a models folder that contains models for brands, categories, etc. The application (eg controllers and HTML views) does all its processing using models (and arrays of models). For some of the record types, there seemed to be references to fields that don't exist in the data - the models behave as if they do exist, so that the application can continue using them. It's just that the values are not populated nor saved. There were also record types that didn't seem to have any data at all (eg certifications). Again, the models/repositories behave as if they do exist, so the application can go through the motions.
When data is saved, it actually saves a model. So if you only want to change one field, you load the model, set the field value, and save the model. This means the model can make sure the whole record is valid before saving.
When reviewing, I would suggest looking through the new branch in isolation, just to understand what it's doing, then look at the comparison with master.
New bugs
After doing my changes, I found a weird bug when "bind"ing firebase to the controller data. If you change an image and save, it seems to lose the bind after uploading the image, and then the other changes doesn't get saved (synchronised). If you make changes and don't change the image, it works. I therefore had to "re-bind" before saving the changes. This produces a nasty looking error in the console, but it still saves the data okay. If anyone knows why this happens and can make a better fix, I would be very happy!
Other Notes
Because the sync processes are asynchronous, I had to pass in a lot of callback functions, so that the calling code could do its "post save" processing at the tight time. If a save process has to save an image and save field changes, you end up with callbacks inside callbacks. It's not very nice, but it's the only way to do it.
Because the "bind" is between firebase and a controller property, there's a slightly yucky dependency between the data repository and the controller - the controllers have to pass $scope into the DataRepositoryFactory::ready() method.
I haven't attempted to write any unit tests, unfortunately (I've never done them in JS, and I ran out of time).
Because a lot of the model have the same core set of properties (and getters and setters), I wanted to create a "super class" with those common things in, and extend it in the models, but I never had time to look at this.