Have one Coupon for each CouponCode #2116

michalrus · 2017-06-09T12:41:39Z

Resolves #1489

So what this does is:

we get rid of two requests to first create a Coupon, and only then some CouponCodes. Now the CreateCoupon payload has two more fields:
- singleCode :: String, if you want a single code
- generateCodes :: ThatPreviousGenerateCodes, which is { prefix :: String, quantity :: Int, length :: Int}, if you want multiple codes.
- And you’re free to use both at the same time. 😊 (But at least one!)
And now, each CouponCode gets its own Coupon,
so the coupon creation endpoint will return a list of what it used to return, one item per coupon (+ code).
Each element of that response will also have a code :: String.
Search views are unchanged, and just work.
No migrations are needed.

@bagratinho, I need your help with redoing that part of Ashes that’s sending the coupon creation request and (possibly) parsing its response. 🙏 📿

Resolves #1489

michalrus · 2017-06-09T12:44:13Z

phoenix-scala/phoenix/app/phoenix/models/coupon/CouponCode.scala

@@ -48,7 +48,7 @@ object CouponCodes
    requestedLimit >= prefixSize + minSuffixSize
  }

-  def generateCodes(prefix: String, codeCharacterLength: Int, quantity: Int, attempt: Int = 0): Seq[String] = {


michalrus · 2017-06-09T12:44:38Z

phoenix-scala/phoenix/app/phoenix/routes/admin/CouponRoutes.scala

-                mutateOrFailures {
-                  CouponManager.getCodes(id)
-                }
+          // TODO: get rid of this route → code is already provided in CouponResponse.Root @michalrus


If it won't be used, then yes

@bagratinho, when tackling this, would you verify that we don’t use

highlander/ashes/src/modules/coupons/details.js

Line 121 in 0f4cc55

return Api.get(`/coupons/codes/${id}`);

and remove it, too?

michalrus · 2017-06-09T12:45:06Z

phoenix-scala/phoenix/app/phoenix/routes/admin/CouponRoutes.scala

-                      CouponManager.update(id, payload, context, auth.model)
-                    }
-                  } ~
+                  // FIXME: unused? @michalrus


@bagratinho tells me we’re not updating coupons. 🔥?

I don't think there's UI for that. @bswineford ?

michalrus · 2017-06-09T12:45:37Z

phoenix-scala/phoenix/app/phoenix/services/coupon/CouponManager.scala

+          CouponResponse.build(context, code, coupon, fas.form, fas.shadow)
+      }
+
+      // TODO: really? Two log events for each coupon code? @michalrus


Probably for coupons and coupon codes separately. I think this can be changed to one activity logging. Same for bulk gen

~~But why does .couponCreated change scope and .singleCouponCodeCreated doesn’t? :<~~

https://github.com/FoxComm/highlander/pull/2116/files#r121897193

michalrus · 2017-06-09T12:47:33Z

phoenix-scala/phoenix/app/phoenix/services/coupon/CouponManager.scala

-      _ ← * <~ CouponCodes.createAll(unsaved)
-      _ ← * <~ LogActivity().multipleCouponCodeCreated(coupon, Some(admin))
-    } yield generated
+      code <- CouponCodes


Oh, cute, git-diff decided we’re copying code enough so that it can show it like that. 😛

michalrus · 2017-06-09T12:48:38Z

phoenix-scala/phoenix/app/phoenix/utils/seeds/generators/CouponGenerator.scala

-                 CouponManager.create(payload, context.name, None).map { newCoupon ⇒
-                   source.copy(formId = newCoupon.id)
+                 CouponManager.create(payload, context.name, None).map { newCoupons ⇒
+                   newCoupons.map(newCoupon => source.copy(formId = newCoupon.id))


Hmm, what do these SimpleModels actually give us, over using just Models?

Apart from increasing the amount of code to maintain, that is. 😛

annappropriate · 2017-06-13T09:45:12Z

phoenix-scala/phoenix/app/phoenix/payloads/CouponPayloads.scala

+                          generateCodes: Option[GenerateCouponCodes])
+
+  // FIXME: unused? @michalrus
+  //case class UpdateCoupon(attributes: Map[String, Json], promotion: Int)


Hm. Seems you can remove it

Let’s wait for @bswineford / @bagratinho, but I don’t see UI for editing coupons either.

Or whatever, I’m removing this.

annappropriate · 2017-06-13T09:45:38Z

phoenix-scala/phoenix/app/phoenix/routes/admin/CouponRoutes.scala

-                mutateOrFailures {
-                  CouponManager.getCodes(id)
-                }
+          // TODO: get rid of this route → code is already provided in CouponResponse.Root @michalrus


If it won't be used, then yes

annappropriate · 2017-06-13T09:45:58Z

phoenix-scala/phoenix/app/phoenix/routes/admin/CouponRoutes.scala

-                      CouponManager.update(id, payload, context, auth.model)
-                    }
-                  } ~
+                  // FIXME: unused? @michalrus


I don't think there's UI for that. @bswineford ?

annappropriate · 2017-06-13T09:46:18Z

phoenix-scala/phoenix/app/phoenix/services/coupon/CouponManager.scala

+                              .insert(couponFormAndShadow.form, couponFormAndShadow.shadow, payload.schema))
+
+      coupons = formsAndShadows.map(
+        fas =>


Needs reformat?

annappropriate · 2017-06-13T09:47:23Z

phoenix-scala/phoenix/app/phoenix/services/coupon/CouponManager.scala

+          CouponResponse.build(context, code, coupon, fas.form, fas.shadow)
+      }
+
+      // TODO: really? Two log events for each coupon code? @michalrus


Probably for coupons and coupon codes separately. I think this can be changed to one activity logging. Same for bulk gen

annappropriate · 2017-06-13T09:48:18Z

phoenix-scala/phoenix/test/integration/testutils/apis/PhoenixAdminApi.scala

-      def generate(code: String)(implicit aa: TestAdminAuth): HttpResponse =
-        POST(s"$rootPrefix/coupons/codes/generate/$formId/$code", aa.jwtCookie.some)
-    }
+    // TODO: unused @michalrus


michalrus · 2017-06-13T13:08:27Z

[@Anna-ZZZ] bugfix PR probably needs an upd to API docs

& an appliance for Bagrat ⇒ https://foxcommerce.slack.com/archives/C1DMVNVLM/p1497361174779856

michalrus · 2017-06-14T09:21:50Z

phoenix-scala/phoenix/app/phoenix/services/coupon/CouponManager.scala

+      }
+
+      _ ← * <~ coupons.headOption.traverse { firstCoupon ⇒
+            // TODO: Should they override scope? (`LogActivity().withScope(scope)`) @michalrus


Why did .couponCreated change scope and .singleCouponCodeCreated didn’t? :<

michalrus · 2017-06-14T10:57:13Z

developer-portal/content/reference/resources/admin_promotions.apib

@@ -20,7 +20,7 @@

 ### View Illuminated [GET /v1/promotions/{context}/{id}/baked]

-Return illuminated version of promotion.
+Return illuminated version of promotion. FIXME: This is very behind right now. @michalrus


michalrus · 2017-06-14T11:05:19Z

Current status: blocked by make up & make provision not working. Cf. https://foxcommerce.slack.com/archives/C4G19344X/p1497429457377966

bagratinho · 2017-06-14T16:26:23Z

#2184 Ashes changes

michalrus · 2017-06-16T11:23:30Z

VM: ~~https://appliance-10-240-0-25.foxcommerce.com/~~ https://appliance-10-240-0-14.foxcommerce.com/

Adjust Ashes for proper coupon creation

annappropriate · 2017-06-22T12:53:34Z

Conflict

michalrus · 2017-06-22T13:18:35Z

When trying to redeploy:

TASK [dev/flyway : Repair Geronimo Database] ***********************************
fatal: [10.240.0.25]: FAILED! => {"changed": true, "cmd": "FLYWAY_HOME=/usr/local/share/flyway-4.0.3 /usr/local/share/flyway-4.0.3/flyway -locations=filesystem:sql/ -configFile=sql/flyway.conf repair", "delta": "0:00:00.298825", "end": "2017-06-22 13:16:18.777771", "failed": true, "rc": 1, "start": "2017-06-22 13:16:18.478946", "stderr": "ERROR: Unable to obtain Jdbc connection from DataSource (jdbc:postgresql://db.service.consul/geronimo?user=geronimo) for user 'geronimo': FATAL: role "geronimo" does not exist", "stdout": "Flyway 4.0.3 by Boxfuse", "stdout_lines": ["Flyway 4.0.3 by Boxfuse"], "warnings": []}

EDIT: twice. I’ll try to disable it.

michalrus · 2017-06-22T13:27:00Z

https://foxcommerce.slack.com/archives/C04BAMY6Q/p1498004943506145

https://foxcommerce.slack.com/archives/C04BAMY6Q/p1497899410611530

Does new appliance base mean, that I have to destroy & up? :(

michalrus · 2017-06-22T14:05:42Z

@alexanderfc, @bagratinho, @Anna-ZZZ, new VM: https://appliance-10-240-0-14.foxcommerce.com/

jmataya

Found a couple things that we should change:

For item qualifiers, give an option for any product to qualify
Change text of "Items for qualify" to "Qualifying items"
This UX feels pretty confusing -- what does it mean?
We don't seem to be forcing a choice on usage rules. What does this state mean?
I set my offer on a promotion to be free shipping, it appears that I'm going to be charged shipping, even though I'm not
If an exclusive promotion is on the cart and I try to add a promotion that is not exclusive, it kicks the exclusive promotion off

We don't necessarily need to fix up all of this, but wanted to highlight the issues.

jmataya · 2017-06-23T18:47:44Z

Also, items based promotions don't seem to be working for me.

bagratinho · 2017-06-26T10:16:47Z

@jmataya I would say we better create new issue(s) for points described above in your comments, IMO that should be tackled separately, not in this PR, as this one is related to coupon creation. Not promo.

michalrus · 2017-06-26T10:19:12Z

Yes, let’s have small diffs, it would be easier!

annappropriate · 2017-06-26T15:33:16Z

@jmataya I created issues for everything you highlighted, except for the last one.

If an exclusive promotion is on the cart and I try to add a promotion that is not exclusive, it kicks the exclusive promotion off

I believe we have decided that user actions of applying coupons are more important than auto-applied promos. Correct me if I'm wrong tho

michalrus · 2017-06-27T07:41:31Z

I remember that, too!

Have one Coupon for each CouponCode

cb8f2a6

Resolves #1489

michalrus added Bug help wanted Team: Core [Hardcore] WIP labels Jun 9, 2017

michalrus requested a review from annappropriate June 9, 2017 12:41

michalrus commented Jun 9, 2017

View reviewed changes

Merge remote-tracking branch 'origin/master' into bugfix/bulk-gen-codes

0f4cc55

annappropriate suggested changes Jun 13, 2017

View reviewed changes

michalrus added 4 commits June 13, 2017 17:13

Remove some unused coupon code

ccedcaf

Merge remote-tracking branch 'origin/master' into bugfix/bulk-gen-codes

486af43

Remove some more

91c5377

Have only one Activity event for bulk creation

06b1433

michalrus commented Jun 14, 2017

View reviewed changes

Update APIB for coupon changes

84207ba

michalrus commented Jun 14, 2017

View reviewed changes

Merge remote-tracking branch 'origin/master' into bugfix/bulk-gen-codes

ba788b1

michalrus and others added 3 commits June 14, 2017 13:30

Fix APIB

30a3932

Fix formatting

af549b6

Adjust Ashes for proper coupon creation

b770948

bagratinho mentioned this pull request Jun 16, 2017

Adjust Ashes for proper coupon creation #2184

Merged

Merge remote-tracking branch 'origin/master' into bugfix/bulk-gen-codes

017b7ad

minor component fix

a0c35a6

michalrus mentioned this pull request Jun 19, 2017

Creating coupon code with duplicate code results in a scary TMI error #2178

Open

length + prefix.length fix

be3c563

annappropriate approved these changes Jun 21, 2017

View reviewed changes

bagratinho and others added 5 commits June 22, 2017 15:29

Merge branch 'master' into bug/coupons-bulk-hgen

1c95544

Review fixes

783c38c

Merge remote-tracking branch 'origin/master' into bugfix/bulk-gen-codes

a2a9eb4

minor fixes

618d92c

Merge pull request #2184 from FoxComm/bug/coupons-bulk-hgen

040f678

Adjust Ashes for proper coupon creation

Merge remote-tracking branch 'origin/master' into bugfix/bulk-gen-codes

733d5a2

jmataya suggested changes Jun 23, 2017

View reviewed changes

jmataya mentioned this pull request Jun 26, 2017

Fix create promotion with defaults discount attrs #1248

Closed

annappropriate merged commit 076d3ef into master Jun 26, 2017

annappropriate deleted the bugfix/bulk-gen-codes branch June 26, 2017 10:49

bagratinho mentioned this pull request Jun 26, 2017

Jeffs comments on promos #2298

Closed

Have one Coupon for each CouponCode #2116

Have one Coupon for each CouponCode #2116

Conversation

michalrus commented Jun 9, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michalrus Jun 13, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michalrus commented Jun 13, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michalrus commented Jun 14, 2017

bagratinho commented Jun 14, 2017

michalrus commented Jun 16, 2017 • edited

annappropriate commented Jun 22, 2017

michalrus commented Jun 22, 2017 • edited

michalrus commented Jun 22, 2017 • edited

michalrus commented Jun 22, 2017

jmataya left a comment

Choose a reason for hiding this comment

jmataya commented Jun 23, 2017

bagratinho commented Jun 26, 2017

michalrus commented Jun 26, 2017

annappropriate commented Jun 26, 2017

michalrus commented Jun 27, 2017

michalrus commented Jun 9, 2017 •

edited

michalrus Jun 13, 2017 •

edited

michalrus commented Jun 13, 2017 •

edited

michalrus commented Jun 16, 2017 •

edited

michalrus commented Jun 22, 2017 •

edited

michalrus commented Jun 22, 2017 •

edited