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

Patch: com.getvero #844

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Patch: com.getvero #844

wants to merge 2 commits into from

Conversation

misterpig
Copy link
Contributor

So the bug exists in the campaign.id field. Turns out the id field of this is an integer unlike the user.id field.
Removing the type field wouldn't impact the adapter as the adapter removes the type field anyway; keeping type field in the schema is redundant as it'll contain NULL for every event after enrichment.
Also increased the campaign.permalink field to accomodate long URLs.

@alexanderdean
Copy link
Member

Thanks @misterpig !

@snowplowcla
Copy link

@misterpig has signed the Software Grant and Corporate Contributor License Agreement

@BenFradet BenFradet added this to the Release 101 milestone Feb 12, 2019
@BenFradet
Copy link
Contributor

@chuwy how should we proceed here? background is in #842

@chuwy
Copy link
Contributor

chuwy commented Feb 14, 2019

Hey @BenFradet, @misterpig,

I still have the same opinion I left in #842:

  • If any amount of data (5/8 as far as I can understand) got to DB, we cannot accept this as a patch because we'll need to migrate the table, otherwise it will just break loading process
  • We can accept it as 2-0-0 without problems
  • Commit history needs to follow ticket:commit 1:1 convention

@chuwy
Copy link
Contributor

chuwy commented Feb 14, 2019

And also as I mentioned before, it seems we're using type property in SCE logic, so I'm not sure we can safely remove it.

@BenFradet
Copy link
Contributor

ok thanks, leaving in the 101 milestone for now.

@misterpig if you could do the necessary changes, i.e. move to 2-0-0, we could integrate it into release 113.

@BenFradet BenFradet mentioned this pull request Feb 14, 2019
10 tasks
@BenFradet BenFradet modified the milestones: Release 101, Release 102 Feb 26, 2019
@BenFradet BenFradet modified the milestones: Release 102, Release 103 May 1, 2019
@BenFradet BenFradet removed this from the Release 103 milestone May 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants