Added membership_plans. Fixes #14 #15

the-metalgamer · 2017-03-03T09:34:12Z

No description provided.

the-metalgamer · 2017-04-24T15:02:26Z

Any update on this?

rnestler · 2017-04-24T22:58:58Z

@dbrgn, @dns2utf8 any opinions? I currently don't have time to look at it. Is this related to #14 ?

dbrgn · 2017-04-25T07:02:25Z

14-draft.json

+                        "monthly",
+                        "weekly",
+                        "daily",
+                        "hourly"


Maybe this should include "other". What if there is biyearly payment or some other type of system?

If renewal interval is "other", then the description must be filled out. (I don't think that's something we can enforce with JSON schema though. But we can document it.)

Might be a good idea! We add it now!

dbrgn · 2017-05-09T12:18:22Z

14-draft.json

@@ -1132,6 +1132,49 @@
          "end"
        ]
      }
+    },
+    "membership_plans": {
+        "description": "A list of the different memberhsip plans your hackerspace might have.",


*membership

dbrgn · 2017-05-09T12:18:51Z

14-draft.json

+                    "type": "string"
+                },
+                "value": {
+                    "description": "How much does this plan cost!",


replace ! with ?

dbrgn · 2017-05-09T12:19:32Z

14-draft.json

+                    "type": "string"
+                },
+                "renewal_interval": {
+                    "description": "When does the membership be renewed! If you select other, please specify in the description what your renewal interval is.",


Rewrite the first sentence to to How often is the membership renewed?

dbrgn · 2017-05-09T12:20:45Z

CHANGELOG.md

@@ -13,3 +13,4 @@ Changes should start with one of the following tags:
 - [added] The `contact.keymasters` array items can also contain a `xmpp` field
 - [changed] The value in `sensors.account_balance` can now be any ISO 4217 string
 - [added] The `timezone` key was added under `location`
+- [added] The `membership_plans` was added to represent membership plans a space might have


Can you add "key" between "membership_plans" and "was"?

the-metalgamer · 2017-05-25T22:22:38Z

Any update on this?

rnestler · 2017-05-26T12:58:12Z

Any update on this?

Sorry for the delay! One doesn't get a notification from Github when you push a commit to a branch. So to avoid us not noticing when you add the requested changes, just add a comment which mentions the reviewer who requested them (@dbrgn in this case).

dbrgn · 2017-05-26T21:19:49Z

Looks good to me now! @rnestler, agree?

rnestler

Some bike-shedding and a more serious question about membership renewal interval vs. payment interval.

rnestler · 2017-05-29T08:03:29Z

14-draft.json

+            "type": "object",
+            "properties": {
+                "name": {
+                    "description": "The name of the membership plan. For example Student Membership, Normal Membership etc...",


Minor nitpick: I would use either etc. or ...:

"The name of the membership plan. For example Student Membership, Normal Membership, ..."

"The name of the membership plan. For example Student Membership, Normal Membership, etc."

rnestler · 2017-05-29T08:19:53Z

14-draft.json

+                    "description": "How often is the membership renewed? If you select other, please specify in the description what your renewal interval is.",
+                    "type": "string",
+                    "enum": [
+                        "yearly",


Is "yearly" or "annually" more common?

I think i would stick with "yearly" since it's more consistent with the other options.

rnestler · 2017-05-29T08:20:46Z

14-draft.json

+                        "weekly",
+                        "daily",
+                        "hourly",
+                        "other"


Do we now which intervals are most common? Maybe add bi-annually? Also at coredump for example we communicate 10CHF/month, but actually bill yearly and also membership is renewed yearly. How would we describe that?

{ "name": "Student", "value": 10, "currency": "CHF", "renewal_interval": "monthly", "description": "Billed annually" }

or

{ "name": "Student", "value": 120, "currency": "CHF", "renewal_interval": "yearly" }

I just see some potential for confusion here.

I guess the renewal and billing usually match, so we should probably chose the yearly renewal interval in our case.

I think we just need to document clearly, how it is intended to be used.

Yes, absolutely :) Any proposals for wording? Maybe How often is the membership renewed (billed)??

I think we could just add some concrete examples to the description string?

@the-metalgamer ok, do you want to do the necessary adjustments?

@dbrgn Yes, but currently I'm thinking about how to incorporate the examples in the descriptions

We can also merge it as is and improve it later, to not stall this PR for such a long time.

That might be a good idea!

ping @rnestler @dbrgn

dbrgn · 2017-06-08T06:38:00Z

Thanks!

Fixes #14.

the-metalgamer added 2 commits March 3, 2017 10:31

Added membership_plans

b09d8a8

Added missing comma

0355f5d

dbrgn requested changes Apr 25, 2017

View reviewed changes

Added other to membership renewal enum

e6e55ef

dbrgn requested changes May 9, 2017

View reviewed changes

Added requested changes

25e45bf

dbrgn approved these changes May 26, 2017

View reviewed changes

dbrgn requested a review from rnestler May 26, 2017 21:19

rnestler reviewed May 29, 2017

View reviewed changes

Use etc. not etc... in membership_plans description

45beec2

dbrgn merged commit 5cd9343 into SpaceApi:master Jun 8, 2017

dbrgn mentioned this pull request Jun 8, 2017

Improve wording for membership_plan renewal #16

Closed

dbrgn pushed a commit that referenced this pull request Jan 6, 2018

Add membership_plans (#15)

ce51a8e

Fixes #14.

Added membership_plans. Fixes #14 #15

Added membership_plans. Fixes #14 #15

Uh oh!

Conversation

the-metalgamer commented Mar 3, 2017

Uh oh!

the-metalgamer commented Apr 24, 2017

Uh oh!

rnestler commented Apr 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

the-metalgamer commented May 25, 2017

Uh oh!

rnestler commented May 26, 2017

Uh oh!

dbrgn commented May 26, 2017

Uh oh!

rnestler left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbrgn commented Jun 8, 2017

Uh oh!

Uh oh!