Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Write data to firestore #169

Merged
merged 16 commits into from
Dec 16, 2019
Merged

Write data to firestore #169

merged 16 commits into from
Dec 16, 2019

Conversation

iritush
Copy link
Collaborator

@iritush iritush commented Dec 7, 2019

@nfloersch @doub1ejack, this is a draft still, but if you have a moment to take a look, let me know what you think so far. I ‘started over’ with this new branch to set things up for writing to the db. I found a cveoeo file with multiple lines of actual data so I used this file to get started.
I ended up using the System ID provided in these files as the uid. I think that if it is in fact a unique id that they use on their end, it might be clearer. In addition, it will simplify things for the (rare) case where a client decides to change the email address they want to use. We can totally change the uid to email if that is preferred.
Next step is to define the goals data and write it as a collection under the user document. I will use the sample data that Nick provided on Tuesday for what fields to have there

In the parseCSVFromServer() function, a new User object is created with the new data.
On the updateOrCreateUseronFireStore() the new data is updated in the user record or a new user is created if that user does not exist
Copy link
Member

@doub1ejack doub1ejack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good @iritush. Good job figuring out how all this works together.

The only code change I would recommend is on line 218 of the index file: removing the currentUser transformation and using your data model directly. This may have downstream affects, but that's ok because (ideally) the models should be the "single source of truth", and the mobile app and the firestore database should reflect the model (not the other way around). But you're more familiar with the implementation than I am so let me know if you see any issues that I have not.

I also added a few non-functional clarifying comments and such. But this looks great. Very excited 🎉

functions/models/user.js Outdated Show resolved Hide resolved
functions/models/validators.js Outdated Show resolved Hide resolved
functions/index.js Outdated Show resolved Hide resolved
functions/index.js Outdated Show resolved Hide resolved
functions/index.js Outdated Show resolved Hide resolved
functions/index.js Outdated Show resolved Hide resolved
iritush and others added 13 commits December 8, 2019 19:53
Co-Authored-By: Micah Mutrux <[email protected]>
Co-Authored-By: Micah Mutrux <[email protected]>
…Firestore database using class methods in the ‘user’ class

-Changed some function and variable names for clarity
-Added comments, TODOs, and did some cleanup
parseCSVAndSaveToFireStore: added reading goal fields from CSV and saving to firestore.
Known issue: When parsing a csv file with multiple lines that have goal data, saving to firestore is not working properly
@iritush iritush marked this pull request as ready for review December 11, 2019 20:32
@iritush
Copy link
Collaborator Author

iritush commented Dec 11, 2019

In this branch:
-Removed updateOrCreateUseronFireStore(). Instead, write data to the Firestore database using class methods in the ‘user’ class
-added a 'goal' class
-Changed some function and variable names for clarity
-Added comments, TODOs, and did some cleanup
-Known issue: When parsing a csv file with multiple lines that have goal data, saving to firestore is not working properly (working on it :-))
TODOs to point out:
-look into tracking the name of the last parsed file and pulling all new files that were
added to the server since then
-confirm with cvoeo that the 'System Name ID' field is a reliable unique id to use

Copy link
Member

@doub1ejack doub1ejack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Irit, thanks so much for your changes. There are some good, functional improvements since the last review, but the comments and todos are almost more important. They really helped make it clearer for me (and others, I'd imagine) to see how things work.

Feel free to merge this whenever you like 🎉 ! Also, when you merge this work, would you also create some new issues on the project board for whatever TODOs you feel are most substantial or important? Thanks for the great work Irit.

@iritush iritush merged commit 12373cc into master Dec 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants