CORE-2016: support scheduled rate and quota default changes. #63

slr71 · 2024-10-29T01:30:50Z

This change allows both rates and default quota values to be updated for a plan. Plan rates are a new feature. The rate associated with the plan is associated with the subscription plan. Multiple rates can be associated with the plan, and the currently active plan rate is always the one with the most recent effective date that is on or before the current date. Plan quota defaults are implemented in the same way, but there's a different active plan quota default for each resource type.

…ate changes

…ription rate changes

…-ons

… be idempotent

…dpoints

…uota-defaults endpoint

…plans/:plan-id/active-quota-defaults endpoints

…nd multiple scheduled plan quota default updates

…-id/rates` endpoint

…d endpoint and did some refactoring

slr71 · 2024-10-30T23:46:45Z

.env.sample

+#!/usr/bin/env bash
+QMS_DATABASE_URI=postgres://de@localhost/qms?sslmode=disable
+QMS_DATABASE_MIGRATE=false
+QMS_DATABASE_REINIT=false
+QMS_USERNAME_SUFFIX=iplantcollaborative.org


I've found direnv to be extremely useful for development testing, so I thought I'd set it up for this repository.

slr71 · 2024-10-30T23:49:33Z

internal/controllers/plans.go

@@ -152,11 +153,10 @@ func (s Server) AddPlan(ctx echo.Context) error {

 			log.Debugf("adding plan quota default resource %s to plan %s", resourceType.Name, plan.Name)
 		}
-
-		log.Debug("adding plan to the database")
+		log.Debugf("translated plan: %+v", dbPlan)


I'm on the fence about leaving this log message in place.

slr71 · 2024-10-30T23:49:51Z

internal/controllers/plans.go

@@ -133,11 +125,20 @@ func (s Server) AddPlan(ctx echo.Context) error {
 	}

 	log = log.WithFields(logrus.Fields{"plan": plan.Name})
-	log.Debug("adding plan to the database")
+	log.Debugf("adding a new plan to the database: %+v", plan)


I'm on the fence about logging all of the plan details in this message.

I think that for this and the other log messages, as long as they're on Debug level anyway, I'm not that worried. They won't be running with that in prod anyway.

slr71 · 2024-10-30T23:57:51Z

internal/db/plan.go

+		Preload("PlanQuotaDefaults", func(db *gorm.DB) *gorm.DB {
+			return db.
+				Joins("INNER JOIN resource_types ON plan_quota_defaults.resource_type_id = resource_types.id").
+				Order("plan_quota_defaults.effective_date asc, resource_types.name asc")
+		}).


The purpose of this is just to set the sort order. I was a little afraid that the join would cause problems with field value resolution, but it seems to be working.

slr71 · 2024-10-31T00:00:03Z

internal/db/plan.go

+		Select("DISTINCT ON (resource_type_id) resource_type_id", "id", "plan_id", "quota_value", "effective_date").
+		Where("effective_date <= CURRENT_TIMESTAMP AND plan_id = ?", planID).


This appears to do what I want it to do, but SQL sanity checks are always a good thing. 😄

I feel like I'm not quite following what this does. I'd expect from the name for it to return only one default for each resource type in the active plan, but it seems like if there were to be two defaults with different effective dates but both in the past, it'd return both of them. But there's possibly something clever I'm missing, heh.

Fortunately, it's only grabbing the most recent one that has an effective date before the current date, which is what I want it to do. If I'm understanding it correctly, it works because the DISTINCT ON forces it to grab only the first matching row satisfying the conditions for each resource type. The sort order then ensures that the most recent quota default for each resource type is the one that gets chosen.

Ah, yes, I see. I confirmed with the PostgreSQL docs and it does explicitly say there that it's the first row and which row is first should be controlled by ORDER BY, so seems perfect.

LGTM now that I wrapped my head around it 😁

slr71 · 2024-10-31T00:07:56Z

migrations/000013_multiyear_subscriptions.down.sql

@@ -6,6 +6,6 @@ BEGIN;

 SET search_path = public, pg_catalog;

-ALTER TABLE resource_types DROP IF EXISTS consumable CASCADE;


I'd forgotten to add the existence check here before.

slr71 · 2024-10-31T00:08:12Z

migrations/000013_multiyear_subscriptions.up.sql

@@ -6,7 +6,7 @@ BEGIN;

 SET search_path = public, pg_catalog;

-ALTER TABLE resource_types ADD IF NOT EXISTS consumable boolean DEFAULT FALSE;


I'd forgotten to add the existence check here before as well.

slr71 · 2024-10-31T00:09:35Z

server/router.go

-	// Lists all of the active user plans.
-	users.GET("/all_active_users", s.GetAllActiveSubscriptions)


This endpoint has never been used as far as I'm aware.

slr71 · 2024-10-31T00:09:57Z

server/router.go

-	// GET /:username/resources/overages returns summaries of any usages that exceed the quota for the corresponding resource.
-	users.GET("/:username/resources/overages", s.GetUserOverages)
-
-	// GET /:username/resources/:resource-name/overage returns whether the usage exceeds the quota for the resource.
-	users.GET("/:username/resources/:resource-name/in-overage", s.InOverage)


The overage endpoints are now implemented in the subscriptions service.

slr71 · 2024-10-31T00:10:39Z

server/router.go

-	// Adds or updates the quota defaults of a plan.
-	plans.POST("/quota-defaults", s.AddPlanQuotaDefault)


The old endpoint to add plan quota defaults was a little clunky, so I reimplemented it.

…endpoints

ianmcorvidae

I don't feel like I totally grok how gorm works, but other than a few comments on style stuff, this appears basically fine to me. I'm not really following the spot you asked for SQL feedback, so I might be able to review that more carefully with a bit more understanding.

ianmcorvidae · 2024-10-31T22:38:13Z

internal/model/plan.go

 }

-// GetDefaultQuotaValue returns the default quota value associated with the resource type with the given name.
+// GetDefaultQuotaValue returns the default quota value associated with the resource type with the given name. This
+// funciton assumes that the plan quota defaults ar sorted in ascending order by effective date.
 func (p *Plan) GetDefaultQuotaValue(resourcetypeName string) float64 {


I'm not sure if this is helpful or not, but it'd at least cut down lines of code if this function was implemented using the newly-added GetDefaultQuotaValues instead. Really the only thing this function is doing differently is a conditional on the resource type name rather than using that name as a key, and I believe it'll look at the same collection of quota defaults since they both just range over it and break when they pass time.Now().

(the comment also has a couple typos, which doesn't really matter at all)

Definitely helpful. Thanks for the suggestion!

ianmcorvidae · 2024-10-31T22:42:30Z

internal/controllers/plans.go

@@ -133,11 +125,20 @@ func (s Server) AddPlan(ctx echo.Context) error {
 	}

 	log = log.WithFields(logrus.Fields{"plan": plan.Name})
-	log.Debug("adding plan to the database")
+	log.Debugf("adding a new plan to the database: %+v", plan)


I think that for this and the other log messages, as long as they're on Debug level anyway, I'm not that worried. They won't be running with that in prod anyway.

ianmcorvidae · 2024-10-31T22:47:45Z

internal/db/plan.go

+		Select("DISTINCT ON (resource_type_id) resource_type_id", "id", "plan_id", "quota_value", "effective_date").
+		Where("effective_date <= CURRENT_TIMESTAMP AND plan_id = ?", planID).


I feel like I'm not quite following what this does. I'd expect from the name for it to return only one default for each resource type in the active plan, but it seems like if there were to be two defaults with different effective dates but both in the past, it'd return both of them. But there's possibly something clever I'm missing, heh.

ianmcorvidae · 2024-10-31T22:49:50Z

internal/db/subscriptions.go

+	currentIndex := 0
+	for _, quotaDefault := range pqds {
 		quotaValue := quotaDefault.QuotaValue
 		if quotaDefault.ResourceType.Consumable {
 			quotaValue *= float64(periods)
 		}
-		result[i] = model.Quota{
+		result[currentIndex] = model.Quota{
 			Quota:          quotaValue,
 			ResourceTypeID: quotaDefault.ResourceTypeID,
 		}
+		currentIndex++


I'm missing if there's a benefit to managing the currentIndex outside of the loop, since it appears to unconditionally increment it at the end of the loop iteration. It seems like this could be done more cleanly with for currentIndex, quotaDefault := range pqds (more similarly to the old code)

The reason in this case is because the plan quota defaults are now in a map to ensure that we only have one plan quota default for each resource type.

I considered making another call to the database to get the active plan quota defaults, which would turn the data structure back into an array, but since we already have everything we need in memory, finding the correct plan quota defaults in code seemed to be the better option just to avoid another round trip to the database. I'm still on the fence about this decision, though. If enough plan quota defaults are added then the time saved by avoiding a round trip to the database might eventually become negligible (or be eliminated entirely).

Ah! That makes sense. I had failed to notice it was a map, and thus wouldn't have indices to use.

To be honest, I'd forgotten about it too and noticed it when I started to update the code. 😄

…uotaValues for added clarity

slr71

Thanks for the review!

slr71 · 2024-11-01T18:40:14Z

internal/db/plan.go

+		Select("DISTINCT ON (resource_type_id) resource_type_id", "id", "plan_id", "quota_value", "effective_date").
+		Where("effective_date <= CURRENT_TIMESTAMP AND plan_id = ?", planID).


Fortunately, it's only grabbing the most recent one that has an effective date before the current date, which is what I want it to do. If I'm understanding it correctly, it works because the DISTINCT ON forces it to grab only the first matching row satisfying the conditions for each resource type. The sort order then ensures that the most recent quota default for each resource type is the one that gets chosen.

slr71 · 2024-11-01T19:00:14Z

internal/db/subscriptions.go

+	currentIndex := 0
+	for _, quotaDefault := range pqds {
 		quotaValue := quotaDefault.QuotaValue
 		if quotaDefault.ResourceType.Consumable {
 			quotaValue *= float64(periods)
 		}
-		result[i] = model.Quota{
+		result[currentIndex] = model.Quota{
 			Quota:          quotaValue,
 			ResourceTypeID: quotaDefault.ResourceTypeID,
 		}
+		currentIndex++


The reason in this case is because the plan quota defaults are now in a map to ensure that we only have one plan quota default for each resource type.

I considered making another call to the database to get the active plan quota defaults, which would turn the data structure back into an array, but since we already have everything we need in memory, finding the correct plan quota defaults in code seemed to be the better option just to avoid another round trip to the database. I'm still on the fence about this decision, though. If enough plan quota defaults are added then the time saved by avoiding a round trip to the database might eventually become negligible (or be eliminated entirely).

slr71 · 2024-12-16T20:42:33Z

Thanks for the review!

slr71 and others added 17 commits October 1, 2024 17:16

CORE-2016: began implementing the database changes for subscription r…

a722913

…ate changes

CORE-2016: finished the initial schema migrations for shceduled subsc…

f5357ab

…ription rate changes

CORE-2016: add a new column to record the rate used for purchased add…

cacafbe

…-ons

CORE-2016: update some existing migrations to make the more likely to…

428efbf

… be idempotent

CORE-2016: set up direnv to simplify local development testing

e59fc0f

CORE-2016: changed money columns to numeric columns in the database

07c021e

CORE-2016: added support for the effective_date field to the plans en…

7466e13

…dpoints

CORE-2016: removed the POST /v1/plans/quota-defaults endpoint

12ea653

CORE-2016: added an endpoint to add quota defaults to an existing plan

ba44de1

CORE-2016: fixed a tpyo

55b6516

CORE-2016: fixed some Swagger warnings

3e4e89b

CORE-2016: fixed a couple of errors in the POST /v1/plans/{plan-id}/q…

dd1172c

…uota-defaults endpoint

CORE-2016: added stubs for three new endpoints

b224beb

CORE-2016: implemented the GET /v1/plans/:plan-id/active-rate endpoint

1c6dfab

CORE-2016: implemented the POST /v1/plans/:plan-id/rates and GET /v1/…

f59ea57

…plans/:plan-id/active-quota-defaults endpoints

CORE-2016: added a missing swagger code annotation

9ec70b4

CORE-2016: updated the subscription endpoints to support plan rates a…

4db2914

…nd multiple scheduled plan quota default updates

slr71 requested review from johnworth, ianmcorvidae, psarando and tedgin October 29, 2024 01:31

slr71 and others added 2 commits October 28, 2024 18:43

CORE-2016: fixed a lint warning

2ea589f

CORE-2016: added unique indexes on plan rates and plan quota defaults

524185a

slr71 force-pushed the main branch from f26ecc4 to 524185a Compare October 29, 2024 20:47

slr71 added 6 commits October 29, 2024 14:11

CORE-2016: removed the all_active_users endpoint

c69bb51

CORE-2016: added duplicate checks for plan quota defaults and plan rates

5d84e2c

CORE-2016: added request body validation to the `POST /v1/plans/:plan…

99476db

…-id/rates` endpoint

CORE-2016: fixed a bug in the PUT /v1/resource-types/:resource-type-i…

fb8dffa

…d endpoint and did some refactoring

CORE-2016: added a response to the Swagger comments

6b9d053

CORE-2016: removed some unused endpoints

fa43dc9

slr71 commented Oct 31, 2024

View reviewed changes

slr71 marked this pull request as ready for review October 31, 2024 00:11

CORE-2016: slight changes to the response body for the subscriptions …

266304a

…endpoints

ianmcorvidae reviewed Oct 31, 2024

View reviewed changes

CORE-2016: reimplemented GetDefaultQuotaValue in terms of GetDefaultQ…

ecc50af

…uotaValues for added clarity

slr71 commented Nov 1, 2024

View reviewed changes

slr71 added 2 commits November 19, 2024 17:13

CORE-2016: fixed a few errors in the Swager docs

2e51283

CORE-2016: fixed the capitalization on one of the JSON fields

9bdd2e1

slr71 merged commit 20f9dc4 into cyverse:main Dec 16, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CORE-2016: support scheduled rate and quota default changes. #63

CORE-2016: support scheduled rate and quota default changes. #63

slr71 commented Oct 29, 2024

slr71 Oct 30, 2024

slr71 Oct 30, 2024

slr71 Oct 30, 2024

ianmcorvidae Oct 31, 2024

slr71 Oct 30, 2024

slr71 Oct 31, 2024

ianmcorvidae Oct 31, 2024

slr71 Nov 1, 2024

ianmcorvidae Nov 1, 2024 •

edited

Loading

slr71 Oct 31, 2024

slr71 Oct 31, 2024

slr71 Oct 31, 2024

slr71 Oct 31, 2024

slr71 Oct 31, 2024

ianmcorvidae left a comment

ianmcorvidae Oct 31, 2024

slr71 Nov 1, 2024

ianmcorvidae Oct 31, 2024

ianmcorvidae Oct 31, 2024

ianmcorvidae Oct 31, 2024

slr71 Nov 1, 2024

ianmcorvidae Nov 1, 2024

slr71 Nov 1, 2024

slr71 left a comment

slr71 Nov 1, 2024

slr71 Nov 1, 2024

slr71 commented Dec 16, 2024

		Select("DISTINCT ON (resource_type_id) resource_type_id", "id", "plan_id", "quota_value", "effective_date").
		Where("effective_date <= CURRENT_TIMESTAMP AND plan_id = ?", planID).

		@@ -6,6 +6,6 @@ BEGIN;

		SET search_path = public, pg_catalog;

		ALTER TABLE resource_types DROP IF EXISTS consumable CASCADE;

		@@ -6,7 +6,7 @@ BEGIN;

		SET search_path = public, pg_catalog;

		ALTER TABLE resource_types ADD IF NOT EXISTS consumable boolean DEFAULT FALSE;

		// Lists all of the active user plans.
		users.GET("/all_active_users", s.GetAllActiveSubscriptions)

		// Adds or updates the quota defaults of a plan.
		plans.POST("/quota-defaults", s.AddPlanQuotaDefault)

CORE-2016: support scheduled rate and quota default changes. #63

CORE-2016: support scheduled rate and quota default changes. #63

Conversation

slr71 commented Oct 29, 2024

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

ianmcorvidae Nov 1, 2024 • edited Loading

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

ianmcorvidae left a 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

slr71 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slr71 commented Dec 16, 2024

ianmcorvidae Nov 1, 2024 •

edited

Loading