Advertisement
Guest User

Untitled

a guest
Sep 20th, 2018
196
0
Never
Not a member of Pastebin yet? Sign Up, it unlocks many cool features!
text 7.36 KB | None | 0 0
  1. [tailsdotcom/tails] Revert "Revert "Feature/international delivery services [WIP]"" (#1307)
  2. Inbox
  3. x
  4.  
  5. (▰˘◡˘▰) <notifications@github.com>
  6. 12:44 (10 hours ago)
  7. to tailsdotcom/tails, Subscribed
  8.  
  9. Reverts tailsdotcom/tails#1306
  10.  
  11. there are bugs in this, and will break production
  12.  
  13. can you also include us in PR's that touch shipment/warehouse code - we are happy to review anything, and offer ideas on how to properly test
  14.  
  15. cheers!
  16.  
  17. You can view, comment on, or merge this pull request online at:
  18. https://github.com/tailsdotcom/tails/pull/1307
  19.  
  20. Commit Summary
  21. Revert "Revert "Feature/international delivery services [WIP]""
  22. File Changes
  23. M docker-compose.yml (20)
  24. M lib/dates.py (5)
  25. M lib/service/delivery.py (95)
  26. M lib/service/geo.py (27)
  27. M lib/shipment/shipment_helper.py (122)
  28. M lib/shipment/yodel/yodel.py (86)
  29. M lib/storage/models/__init__.py (30)
  30. M lib/tasks/order.py (4)
  31. A migrations/versions/fb3cb15c295_add_international_support_to_delivery_.py (124)
  32. M warehouse/admin/views/package.py (7)
  33. M warehouse/admin/views/printer.py (7)
  34. M warehouse/common/utils/package_helper.py (8)
  35. M web/forms/signup.py (1)
  36. M web/forms/validators.py (1)
  37. D web/templates/signup/delivery_service.html (47)
  38. M web/views/account/settings.py (7)
  39. M web/views/api.py (6)
  40. M web/views/signup.py (121)
  41. Patch Links:
  42. https://github.com/tailsdotcom/tails/pull/1307.patch
  43. https://github.com/tailsdotcom/tails/pull/1307.diff
  44. You are receiving this because you are subscribed to this thread.
  45. Reply to this email directly, view it on GitHub, or mute the thread.
  46.  
  47.  
  48. Matt M <notifications@github.com>
  49. 15:29 (7 hours ago)
  50. to tailsdotcom/tails, Subscribed
  51.  
  52. Hey, have you got any info on what the issue was? Changes to shared code were pretty minimal. We need this live asap
  53.  
  54.  
  55.  
  56. (▰˘◡˘▰)
  57. 15:56 (7 hours ago)
  58. well, your change may have been minimal - but it broke production : )
  59.  
  60. Matt M
  61. 16:00 (7 hours ago)
  62. Understood, do you know what the issue is or do you have any logs I can take a look at to help me figure out what the issue is?
  63.  
  64. Steve Webster <notifications@github.com>
  65. 18:11 (5 hours ago)
  66. to tailsdotcom/tails, Subscribed
  67.  
  68. I agree with @NULL-OPERATOR's point around PRs for shared code, but since that ship has sailed for this change I want to focus on getting this fixed as it's a key business priority. Since you're both going to be in Richmond tomorrow, can you please both sit down, work out what's broken and collaborate on the best way to fix it. Tomorrow is Friday so we won't now be able to push this change live ahead of the weekend, but it would be great to be in a position to go live on Monday next week.
  69.  
  70.  
  71.  
  72. Matt M
  73. 20:31 (2 hours ago)
  74. @zelane pushed 1 commit. - a3d5b85 rename france store to fr for consistency View it on GitHub or mute the thread.
  75.  
  76. (▰˘◡˘▰) <notifications@github.com>
  77. 21:25 (1 hour ago)
  78. to Subscribed, tailsdotcom/tails
  79.  
  80. has anyone run this code yet?
  81.  
  82.  
  83.  
  84. Matt M <notifications@github.com>
  85. 21:28 (1 hour ago)
  86. to tailsdotcom/tails, Subscribed
  87.  
  88. it's be run against, web, fy and by. We don't currently have any test setup for packing etc or any docs on it. If I can get some kind of logs for the issue that was seen I can implement a fix and we can test it together tomorrow. Whilst digging in to this it looks like the migration in this PR wasn't run against the prod db, so maybe that was the issue?
  89.  
  90.  
  91.  
  92. (▰˘◡˘▰) <notifications@github.com>
  93. 21:53 (1 hour ago)
  94. to tailsdotcom/tails, Subscribed
  95.  
  96. but why are you editing code you do not know how to run?
  97. not running it..
  98. and then pushing it to live?
  99.  
  100. you can always chuck anything you need doing on our end into our sprint (which lives on trello, and everyone should have access to?), you've just got to turn up and bargain for priority against the rest of them haha, and if you ever want any of us to run through or help you setup stuff, we are always here to help! (this goes for anyone else too!)
  101.  
  102. I can help you run the code tomorrow, I will come find you and we can do it on your laptop : )
  103.  
  104.  
  105.  
  106. (▰˘◡˘▰) <notifications@github.com>
  107. 21:57 (1 hour ago)
  108. to tailsdotcom/tails, Subscribed
  109.  
  110. @NULL-OPERATOR commented on this pull request.
  111.  
  112. In lib/shipment/shipment_helper.py:
  113.  
  114. > - '10': 1, #before 10 30
  115. - '12': 1, #before 12
  116. - '48': 2,
  117. - '72': 3,
  118. - 'packet': 3,
  119. - 'ISLE': 5}
  120. +# TODO: This could move to the CourierDeliveryService?
  121. +fulfilment_days = {
  122. + '24': 1,
  123. + '10': 1, # before 10 30
  124. + '12': 1, # before 12
  125. + '48': 2,
  126. + '72': 3,
  127. + 'packet': 3,
  128. + 'ISLE': 5,
  129. + 'international': 5
  130. also.. where was this decided?
  131.  
  132.  
  133.  
  134. Matt M <notifications@github.com>
  135. 22:13 (1 hour ago)
  136. to tailsdotcom/tails, Subscribed
  137.  
  138. It's shared code so it's run in several applications, it's been tested in the ones I have a running environment for in development, which did not have any issues.
  139.  
  140. Given that the only changes that should impact other services are the changes to two function signatures, and I searched the code base for and updated the call sites, I did not consider it high enough risk to trade off against a full regression costing both of us time, given that the plan was to merge all of our international changes into master and then run a full end to end test together, which should have shaken any issues out before going to prod.
  141.  
  142. These changes were not intended for prod and no one on our side deploy them, however they were merged into master to facilitate the end to end test that everyone was hoping to do today.
  143.  
  144. I really would have appreciated some logs from the issue you saw when it got accidentally pushed to production as it would have let me get ahead of the curve on resolving this issue, which might be as simple as just making sure the migration has been run against prod. As you know the deadline for getting intentional live is pretty punchy and whilst discussions about shared code etiquette are something we should definitely make time for, they will not solve the immediate issue at hand.
  145.  
  146.  
  147.  
  148. Matt M <notifications@github.com>
  149. 22:16 (1 hour ago)
  150. to tailsdotcom/tails, Subscribed
  151.  
  152. @zelane commented on this pull request.
  153.  
  154. In lib/shipment/shipment_helper.py:
  155.  
  156. > - '10': 1, #before 10 30
  157. - '12': 1, #before 12
  158. - '48': 2,
  159. - '72': 3,
  160. - 'packet': 3,
  161. - 'ISLE': 5}
  162. +# TODO: This could move to the CourierDeliveryService?
  163. +fulfilment_days = {
  164. + '24': 1,
  165. + '10': 1, # before 10 30
  166. + '12': 1, # before 12
  167. + '48': 2,
  168. + '72': 3,
  169. + 'packet': 3,
  170. + 'ISLE': 5,
  171. + 'international': 5
  172. the delivery range is arbitrary for now, to be finalised before any international shipments make it into the queue. It simply needs to be there before we can do an end to end test
  173.  
  174.  
  175.  
  176. Steve Webster <notifications@github.com>
  177. 22:20 (1 hour ago)
  178. to tailsdotcom/tails, Subscribed
  179.  
  180. I've cleaned up this PR and left only comments relevant to the change and less on ways of working, which we'll tackle elsewhere. Please keep focussed on solving the issue here.
  181.  
  182.  
  183.  
  184. (▰˘◡˘▰) <notifications@github.com>
  185. 22:47 (33 minutes ago)
  186. to tailsdotcom/tails, Subscribed
  187.  
  188. and that is ok?
  189.  
  190.  
  191.  
  192. (▰˘◡˘▰) <notifications@github.com>
  193. 23:02 (19 minutes ago)
  194. to tailsdotcom/tails, Subscribed
  195.  
  196. I have cleaned up further! looking good!
Advertisement
Add Comment
Please, Sign In to add comment
Advertisement