Click views #31
Click views #31
Conversation
…he firebase collections now being strings, not integers.
…hen the user wants to add another record after having created one.
|
See also the issue that this PR is for. I have refactored the code to save a single record instead of the entire database when anything is changed. This involved rationalising the various ways the application saves data, so now it uses $add(), $save() and $remove(). As a result, the record collection keys are now strings instead of integers - I believe is the correct way of doing it. It no longer uses the syncObject (well almost never). An added bonus is that there are essentially no errors in the console now (there are one or two, but they're not related to saving/loading data). There are a lot of controllers where 1 line ("$scope") has been removed. app/scripts/services/DataRepositoryFactory.js has had a lot of redundant code removed. The main changes are in app/scripts/services/DataRepository.js. I made a couple of small improvements/changes whilst doing this:
|
| @@ -289,6 +315,42 @@ var DataRepository = function (dataContainer, syncObject, recordSets) { | |||
| return id; | |||
| }; | |||
|
|
|||
| this.determineIndexFromBrandModel = function determineIndexFromBrandModel(brandModel) { | |||
| // Determine the index of the firebase array, using the brand model's id. | |||
| var indexToDelete = null; | |||
There was a problem hiding this comment.
Are there any other places we may want to use this function? If there are we may want to rename this variable so it isn't related to deletion. Maybe something like targetIndex or indexMatch or something else completely :) . (same in the other 2 functions below)
There was a problem hiding this comment.
Ah yes, you're right. Well spotted! I re-used the code that was explicitly for deleting a record, but didn't notice the variable name.
I'll change it (and the other methods) now - this method is also used when updating a record.
There was a problem hiding this comment.
I'm afraid I went with a different format for the name :-)
I went with brandIndex, categoryIndex, etc.
The commit is here.
| // Overwrite the record with the values from the model. | ||
| this.populateRecordFromBrandModel(brandSource, brand); | ||
| // Save the record back to the store. | ||
| this.update('brands', brandIndex, successMsg, callback); |
There was a problem hiding this comment.
These comments are exceptionally helpful in keeping how firebase functions straight 👌. Thank you for them!!
There was a problem hiding this comment.
Ah, thank you. Yeah, I put the comments in because when I looked at the code it looked really complicated and it wasn't clear at all what was going on, so I essentially commented every line!
| productRecord.parentCategoryId = product.getParentCategoryId(); | ||
| productRecord.purchaseURlClicks = product.getPurchaseUrlClicks(); | ||
| productRecord.purchaseUrl = product.getPurchaseUrl(); | ||
| // productRecord.$$hashKey = product.$$hashKey(); @TODO: Do we save this? When does it change? |
There was a problem hiding this comment.
My understanding was that this was some internal key to firebase that we don't need to worry about (still very worth looking into, because I'm not sure I'm remembering that correctly or if it's just wishful thinking 😄).
There was a problem hiding this comment.
Haha! Wishful thinking it a dangerous thing!!
I'll leave it for now, but make a note to look into it. Is this a feature of firebase (ie does it generate this hash itself?)
|
@toriejw , in reply to your question about the |
|
Oh, @toriejw , I was also going to say that I am worried/annoyed that there is still quite a lot of duplicated (well very similar) code after my big refactor. You'll learn that one of my pet hates is duplicated code!! |
|
Makes senes on the duplication. I don't have any other ideas currently, but I'll keep it in the back of my head and see if anything comes up :) . |
No description provided.