Skip to content

SAN-4053 - User Engagement Tracking#18

Merged
Myztiq merged 3 commits intomasterfrom
SAN-4053-user-tracking
Apr 28, 2016
Merged

SAN-4053 - User Engagement Tracking#18
Myztiq merged 3 commits intomasterfrom
SAN-4053-user-tracking

Conversation

@Myztiq
Copy link
Copy Markdown
Contributor

@Myztiq Myztiq commented Apr 26, 2016

Added ownerUsername to the naviEntry so we can do tracking on this.

@runnabot
Copy link
Copy Markdown

The latest push to PR-18 is running on SAN-4053-user-tracking-link

@rsandor
Copy link
Copy Markdown
Contributor

rsandor commented Apr 27, 2016

I see no functional testing to ensure the new field is actually being put into to the database.

@Myztiq
Copy link
Copy Markdown
Contributor Author

Myztiq commented Apr 27, 2016

@rsandor Updated functional test to check this 👍

@rsandor
Copy link
Copy Markdown
Contributor

rsandor commented Apr 27, 2016

LGTM

type: Number,
required: true
},
ownerUsername: {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this needs db migration right?

@anandkumarpatel
Copy link
Copy Markdown
Contributor

Looking good, you added a new required field so we need to run a db migration right

@Myztiq
Copy link
Copy Markdown
Contributor Author

Myztiq commented Apr 27, 2016

No db migration needed. It'll all eventually work. This is just for reporting, whenever an update happens it'll have this field.

@anandkumarpatel
Copy link
Copy Markdown
Contributor

anandkumarpatel commented Apr 27, 2016

@Myztiq do you query for NaviEntry? If you do I think mongoose will not return anything that does not fit the schema?

@Myztiq
Copy link
Copy Markdown
Contributor Author

Myztiq commented Apr 27, 2016

@anandkumarpatel Nope, we only upsert to my knowledge. So we don't have to worry about this.

@anandkumarpatel
Copy link
Copy Markdown
Contributor

LGTM

@Myztiq Myztiq merged commit 2581af2 into master Apr 28, 2016
@Myztiq Myztiq deleted the SAN-4053-user-tracking branch April 28, 2016 20:08
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.

4 participants