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

Update Gratipay badge for API change #533

Merged
merged 1 commit into from
Sep 12, 2015
Merged

Update Gratipay badge for API change #533

merged 1 commit into from
Sep 12, 2015

Conversation

dougwilson
Copy link
Contributor

This updates the logic for the Gratipay badge for a change in the Gratipay API where the "receiving" field was renamed to "taking" as part of their migration to teams.

@espadrine
Copy link
Member

As discussed here, taking is not quite the same thing as what receiving showed, as it does not apply to teams. The issue for it on Gratipay's side is gratipay/gratipay.com#3726, and the target is gratipay/gratipay.com#3721.

Given that, it may be more future-proof to have if (receiving) receiving else taking (pseudocode)?

@dougwilson
Copy link
Contributor Author

Yea, I was thinking about that, and it seems like when they finally finalize their API to distinguish teams/people, perhaps Shields wouldn't even support /gratipay/{user} ? Probably would be nice to have two: /gratipay/user/{user} and /gratipay/team/{team}, which would also start the dropping of the old gittip URL here as well. Because of that, I was thinking of just getting /gratipay/{user} to just continue to work, though only for users.

I was actually thinking we should just change the API URL to var apiUrl = 'https://www.gratipay.com/~' + user + '/public.json'; (adding the ~) to this end. Thoughts? This, to me, seems like a way forward for the user/team split as well as continuing to at least keep the user badges functioning?

@espadrine
Copy link
Member

I won't deny I was leaning on the separation of user and team as well! However, I feel we should only separate them if they live on separate namespaces, that is, if we can theoretically have a team with the exact same name as a user. I currently believe that is not possible, so I'd wait to separate them until it becomes possible.

Until then, checking for receiving first and taking if receiving is absent seems safest.

@dougwilson
Copy link
Contributor Author

Gotcha. My understanding of their discussions in their issues was that it would be possible, which is why they added the ~ to the URLs to be able to distinguish.

@espadrine
Copy link
Member

In that case, let's separate them!

Given what is currently implemented, we can set the old shields URL to the behavior of users.

What do you think about something like that?

diff --git a/server.js b/server.js
index 04f9a4f..4856f99 100644
--- a/server.js
+++ b/server.js
@@ -819,10 +819,10 @@ cache(function(data, match, sendBadge, request) {
 }));

 // Gratipay integration.
-camp.route(/^\/(gittip|gratipay)\/(.*)\.(svg|png|gif|jpg|json)$/,
+camp.route(/^\/(?:gittip|gratipay)(?:\/user)?\/(.*)\.(svg|png|gif|jpg|json)$/,
 cache(function(data, match, sendBadge, request) {
-  var user = match[2];  // eg, `JSFiddle`.
-  var format = match[3];
+  var user = match[1];  // eg, `dougwilson`.
+  var format = match[2];
   var apiUrl = 'https://www.gratipay.com/' + user + '/public.json';
   var badgeData = getBadgeData('tips', data);
   if (badgeData.template === 'social') {
@@ -836,8 +836,9 @@ cache(function(data, match, sendBadge, request) {
     }
     try {
       var data = JSON.parse(buffer);
-      if (data.receiving) {
-        var money = parseInt(data.receiving);
+      var receiving = data.receiving || data.taking;
+      if (receiving) {
+        var money = parseInt(receiving);
         badgeData.text[1] = '$' + metric(money) + '/week';
         if (money === 0) {
           badgeData.colorscheme = 'red';

@dougwilson
Copy link
Contributor Author

Awesome, let's iterate! So what I did was took your patch there and altered it a bit as well. My additions were the following:

  • Added the ~ to keep it only functioning for users at the current addresses, which will let anyone using /gratipay/foo.svg to always refer to the user foo even if there is eventually a team called foo.
  • Only allowed the new /user/ path segment on the /gratipay/, to encourage a phase-out of the /gittip/ URLs (unless you don't mind having it).
diff --git a/server.js b/server.js
index aa97ebf..a1f181b 100644
--- a/server.js
+++ b/server.js
@@ -819,11 +819,11 @@ cache(function(data, match, sendBadge, request) {
 }));

 // Gratipay integration.
-camp.route(/^\/(gittip|gratipay)\/(.*)\.(svg|png|gif|jpg|json)$/,
+camp.route(/^\/(?:gittip|gratipay(?:\/user)?)\/(.*)\.(svg|png|gif|jpg|json)$/,
 cache(function(data, match, sendBadge, request) {
-  var user = match[2];  // eg, `JSFiddle`.
-  var format = match[3];
-  var apiUrl = 'https://www.gratipay.com/' + user + '/public.json';
+  var user = match[1];  // eg, `dougwilson`.
+  var format = match[2];
+  var apiUrl = 'https://www.gratipay.com/~' + user + '/public.json';
   var badgeData = getBadgeData('tips', data);
   if (badgeData.template === 'social') {
     badgeData.logo = badgeData.logo || logos.gratipay;
@@ -836,8 +836,9 @@ cache(function(data, match, sendBadge, request) {
     }
     try {
       var data = JSON.parse(buffer);
-      if (data.receiving) {
-        var money = parseInt(data.receiving);
+      var receiving = data.receiving || data.taking;
+      if (receiving) {
+        var money = parseInt(receiving);
         badgeData.text[1] = '$' + metric(money) + '/week';
         if (money === 0) {
           badgeData.colorscheme = 'red';

Thoughts?

@espadrine espadrine merged commit 0bf5dc3 into badges:master Sep 12, 2015
@espadrine
Copy link
Member

My thoughts are that it is great! ☺

@dougwilson
Copy link
Contributor Author

Awesome :) I'm really looking forward to Gratipay having an API for the teams soon :)

@paulmelnikow paulmelnikow mentioned this pull request Apr 2, 2017
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.

2 participants