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

Shipment Creator Bin Command #210

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

Conversation

jamesfneyer
Copy link
Contributor

This branch adds a new bin command to transmission that allows for the automatic creation of shipments. The requires engine, profiles and transmission to all be running locally, and will by default create a new storage credential, shipper/carrier wallet and create 10 new shipments with them.

The creator can be customized by adding in the optional arguments:

optional arguments:
  -h, --help            show this help message and exit
  --total TOTAL, -t TOTAL         Total amount of shipments to be created, defaults to 10
  --startnumber STARTNUMBER, -n STARTNUMBER       Number to start at for sequential attributes
  --partition PARTITION, -p PARTITION         Number shipments to create per wallet, defaults to 10
  --carrier CARRIER, -c CARRIER         Set carrier wallet owner (defaults to user 1)
  --shipper SHIPPER, -s SHIPPER         Set shipper wallet owner (defaults to user 1)
  --moderator MODERATOR, -m MODERATOR         Set moderator wallet owner (defaults to None)
  --verbose, -v         More descriptive when running functions.
  --device, -d          Add devices to shipment creation, defaults to false
  --attributes ATTRIBUTES, -a ATTRIBUTES         Attributes to be sent in shipment creation (defaults to required values)
  --add_tracking        Adds tracking data to shipments (requires --device or -d)
  --add_telemetry       Adds telemetry data to shipments (requires --device or -d)

@jamesfneyer jamesfneyer requested a review from mlclay June 8, 2020 13:32
Copy link
Contributor

@mlclay mlclay left a comment

Choose a reason for hiding this comment

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

Overall conceptually this looks okay. But I see room for improvements in several areas related to program structure, logging, error handling, and argument parsing. Please review the comments below in full before starting to work on any changes as some of them imply changes to other areas. Additionally some of these comments apply to a type of scenario that occurs multiple times, yet the comment only exists once. As you review the comments against your code, be cognizant of similar sections of code and make the appropriate updates there as well.

bin/dev-tools/create_shipments.py Outdated Show resolved Hide resolved
bin/dev-tools/create_shipments.py Outdated Show resolved Hide resolved
bin/dev-tools/create_shipments.py Outdated Show resolved Hide resolved
bin/dev-tools/create_shipments.py Outdated Show resolved Hide resolved
bin/dev-tools/create_shipments.py Outdated Show resolved Hide resolved
bin/dev-tools/create_shipments.py Outdated Show resolved Hide resolved
bin/dev-tools/create_shipments.py Outdated Show resolved Hide resolved
bin/dev-tools/create_shipments.py Outdated Show resolved Hide resolved
bin/dev-tools/create_shipments.py Outdated Show resolved Hide resolved
bin/dev-tools/create_shipments.py Outdated Show resolved Hide resolved
Copy link
Contributor

@mlclay mlclay left a comment

Choose a reason for hiding this comment

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

The exception handling looks better here. When I run this and encounter an error, there are multiple instances of every error message logged to the console. Can you take a look in to this? I think there is something going on with when you are logging various things relating to the new exception handling.

bin/dev-tools/create_shipments.py Outdated Show resolved Hide resolved
bin/dev-tools/create_shipments.py Outdated Show resolved Hide resolved
@ajhodges
Copy link
Contributor

ajhodges commented Jul 8, 2020

Here are my notes from testing as a user - I haven't looked at the code at all yet.

First time I tried running the command, expecting to see some help text. This is what I got instead:
image

As a dumb user, I have no idea what either of those messages mean. As an Adam, I know to start profiles, so I did that next.
image

Better error this time, but it still doesn't let me know what's going on. As an Adam, I know to start engine.
image

As a dummy, no clue what this means. As an Adam, I guess I have to start Transmission independent of this script.

I know that you mentioned in this PR that all three must be running locally - but this isn't immediately obvious without reading your PR, which will fade into obscurity once this is merged.

The next time I ran it:
image

No output, no feedback, no nothing. Did it work? What got created? There were no progress indicators either, it just hung for a few seconds. As an Adam, I know that I can tab over to the Transmission logs and see that a bunch of stuff got created.

As a dumb Adam, reading through the help, I am thoroughly confused by a few of the arguments. I don't know what any of these mean from the name:

  • total (rename this to count or something)
  • startnumber (?? what is a sequential attribute)
  • partition (what is 'partition'? also if total is 10, and part is 10, does this mean only one wallet is used? how do these two parameters interact?)
  • you pass in the username to select the wallet for shipper/carrier/moderator? how does it choose which wallet to use for a username?
  • device (do I pass in a device ID? does a device get created?)
  • more concrete examples of attributes should be provided, including examples where multiple attributes are specified

Other questions I have as a user:

  • which wallets get used? can I change this?
  • which storage credentials get used? can I change this?

Just my initial thoughts - I think there are a lot of improvements that can be made to the UI here, to help make some of these things clear to dumb Adams like myself.

@jamesfneyer
Copy link
Contributor Author

which wallets get used? can I change this?
which storage credentials get used? can I change this?

Wallets are created specifically for this test. Same with storage credentials and devices. Lucas talked about possibly reusing extant wallets/credentials but we hadn't fully decided on including that. I could possibly include such a field though I'd want to think about how it grabs these reusable assets.

that's why you pass in the username to select the wallet for shipper/carrier/moderator since they are all created in the moment. You are passing in the user who shipper/carrier/moderator wallet USER not the wallet itself.

@ajhodges
Copy link
Contributor

New error:
image

Copy link
Contributor

@ajhodges ajhodges left a comment

Choose a reason for hiding this comment

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

Rather than reusing static test data (i.e. same tracking data duplicated x times), it might be a nice addition to make use of a random data generator. Check out the Python faker and factory_boy libraries for some neat tools that can help generate random values. mimesis looks good too, I just found that one. For example, you could have faker generate each subsequent tracking data point within a certain radius of the previous one. You'd probably end up with really zigzaggy data, but it should be fine for testing purposes.

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.

3 participants