-
Notifications
You must be signed in to change notification settings - Fork 286
Add information about order's transactions #15
base: master
Are you sure you want to change the base?
Conversation
Thanks @8geonirt -- will review this as soon as I have the opportunity. |
@t-kelly thank you so much, please let me know if you think that more information needs to be shown in the table. |
Hi @8geonirt, thanks a lot for creating this - sorry for the delay! Haven't had time to sit down and check this out. Quite a bit has been added to Starter Theme since you created this PR back in March. Would you mind either rebasing the latest updates off the |
@chrisberthe Hey Chris, thank you for checking this out, yeah, I can do that, I'll do it in a bit |
- Add information about order's transactions by showing a table with information about each transaction. - Add en translations for titles in transactions' columns More about transactions object in liquid: https://help.shopify.com/themes/liquid/objects/transaction
aaf2836
to
161abb8
Compare
@chrisberthe I've rebased the branch I created with master |
.travis.yml
Outdated
@@ -5,6 +5,8 @@ cache: | |||
yarn: true | |||
directories: | |||
- node_modules | |||
env: | |||
- NODE_ENV=test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mind explaining why the tests were failing without this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chrisberthe Tests were failing because of the slate-analytic
package, it is trying to search for the NODE_ENV
var, if it is not present then slate-analytic
is going to prompt for an email just to have some analytics about Slate. It fails on Travis because Travis CI is waiting for the user's email input.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's interesting - we have a SLATE_USER_EMAIL
env variable already set on Travis CI's side to bypass that prompt. This should have worked after you rebased from master
.
Sorry for the extra work but would you mind removing this from .travis.yml
and seeing if it still fails?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chrisberthe Oh, that's weird, I've rebased the branch with the upstream/master branch, no changes were detected since I did the same the last week, I removed the env var from the travis.yml file and the tests failed again because of the email prompt timeout.
I was looking for SLATE_USER_EMAIL
in the code but I couldn't find anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's something you set in Travis CI so you wouldn't see it in the code. We're currently doing this for SLATE_USER_EMAIL
. No idea why it's not fixing itself :')
We should technically see the following in the build but it's not there:
Setting environment variables from repository settings
$ export SLATE_USER_EMAIL=[secure]
^ This was grabbed from a previous PR that was merged.
Thanks @8geonirt, will have a look soon. |
This fixes #2
- Add information about order's transactions by showing a table with information about each transaction.
- Add english translations for titles in transactions' columns
More about transactions object in liquid: https://help.shopify.com/themes/liquid/objects/transaction