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

Add shop info to the records #73

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

EmaLoprevite
Copy link

@EmaLoprevite EmaLoprevite commented Nov 19, 2020

Description of change

Add shop info to the records to be able to easily distinguish them among multiple shops.

QA steps

  • automated tests passing: the tests include packages (e.g. tap_tester) not publicly available so it is difficult to check them or create new ones;
  • manual qa steps passing (list below): sorry, I am not sure where these steps are listed.

Risks

  • None, because the three new "sdc" fields appears in the output only if they are selected in the catalog.

Rollback steps

  • Revert this branch.

Add shop info to the records to be able to easily distinguish them among multiple shops
@briansloane
Copy link

@EmaLoprevite So that I understand better, can you explain how you would have multiple shop data in one extraction which makes this necessary? I thought that the tap will only extract data for one shop/subdomain.

@EmaLoprevite
Copy link
Author

Hi @briansloane, thank you very much for reviewing my PR.
This change is very useful when downloading data from multiple shops and putting everything into a single table via a SQL target; this way the data can be later filtered by shop, otherwise there's no way to know to which shop a specific row in a table belongs.
Of course, that field can be added/selected via the catalog so it doesn't affect the current behaviour.
If you have other questions, please feel free to ask; thank you again.

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