Implement #81 Sync Script.about with Github webhook

[Zren]

See: #81

Test

https://nameless-hollows-5487.herokuapp.com
Use http://nameless-hollows-5487.herokuapp.com/github/hook for your webhook.

Summary

• Refactored the entire github hook, and parts of github import.

• Webhooks now return a status code on error, and return a [err, jsFileList, mdFileList] response on success. Eg:

  [null, [],
  [{
      "markdownBlob": {
          "path": "description.md",
          "mode": "100644",
          "type": "blob",
          "sha": "eff9a9a548a9017a45cb82a0f6562fddaf2e7aea",
          "size": 63,
          "url": "https://api.github.com/repos/ZrenTest/TestUserScript/git/blobs/eff9a9a548a9017a45cb82a0f6562fddaf2e7aea",
          "canUpload": true,
          "errors": []
      },
      "numScriptsAffected": 1
  }]
  ]

• Moved both controllers to their own files in a pattern I plan to expand to other controllers.

• Refactor /model/settings.json into /config.js which allows us to attach comments, parse env vars and provide defaults for them, and centralize all app variables.

• Rename /user/* routes to /account/*.

• Move express-minify to optionalDependencies. Do not minify when isDbg.

Screenshots

[jerone]

+1 on moving settings to config.js, allowing comments and logic.
+1 on moving sync routes from /user/ to /account/.
-0 on moving express-minify to optionalDependecies (not going to burn my hands)
+1 on adding readme sync (issues fixed).

I do have some problems on getting this sync to work. The hook is now attached (probably want to make the hook url on the add script page dynamic), and added the readme file to my test script, but no readme shows up.

EDIT:
Oke, above was my bad, had used the wrong path. I do think this could be a whole lot easier by automatic looking in the same directory as the userscript is synced from. But that's probably another issue.

[Zren]

All fields are case sensitive. Which probably should be insensitive since the user might type it out.

[Zren]

However, the file path might have to stay case sensitive since

https://github.com/jerone/UserScripts/blob/master/PDF_Tools/README.md = 200
https://github.com/jerone/UserScripts/blob/master/PDF_Tools/readme.md = 404

[jerone]

@Zren commented on 23 nov. 2014 12:09 CET:

However, the file path might have to stay case sensitive since

https://github.com/jerone/UserScripts/blob/master/PDF_Tools/README.md = 200
https://github.com/jerone/UserScripts/blob/master/PDF_Tools/readme.md = 404

Case sensitive it is.

---

The edit description page might benefit from a message saying that every changes done to the description on OUJS will be overridden when the hooked readme file changes. I might be even better to disable the description field when sync is enabled.

[Zren]

Tempted to hook the 'change' event and show a checkmark/red X when we get a 200/404 with:

$.ajax({type:'HEAD',url:'https://cdn.rawgit.com/jerone/UserScripts/master/PDF_Tools/README.md'}).done(function(data, textStatus, res){console.log(res.status)}).fail(function(res, textStatus, errorThrown){console.log(res.status)});

Of course, we'd have to use rawgit.com for to get around the crossdomain issue.

Edit

Don't think I'll use it unless we get a proper 404 testing service. Might instead create a clickable link below the text fields to manually test.

var inputs = $('input[name="githubSyncUserId"], input[name="githubSyncRepoName"], input[name="githubSyncAboutPath"]');
inputs.on('change', function() {
  var githubSyncUserId = $('input[name="githubSyncUserId"]');
  var githubSyncRepoName = $('input[name="githubSyncRepoName"]');
  var githubSyncAboutPath = $('input[name="githubSyncAboutPath"]');
  if (githubSyncUserId && githubSyncRepoName && githubSyncAboutPath) {
    githubSyncUserId = githubSyncUserId.val();
    githubSyncRepoName = githubSyncRepoName.val();
    githubSyncAboutPath = githubSyncAboutPath.val();
    var url = 'https://cdn.rawgit.com/' + githubSyncUserId + '/' + githubSyncRepoName + '/master/' + githubSyncAboutPath;

    $.ajax({
      type:'HEAD',
      url: url
    }).done(function(data, textStatus, res) {console.log(res.status);
      $('#sync-about-info').removeClass('alert-info alert-success alert-danger').addClass('alert-success');
    }).fail(function(res, textStatus, errorThrown) {console.log(res.status);
      $('#sync-about-info').removeClass('alert-info alert-success alert-danger').addClass('alert-danger');
    });
  }
});

[jerone]

@Zren commented on 23 nov. 2014 12:36 CET:

Tempted to hook the 'change' event and show a checkmark/red X when we get a 200/404 with:

$.ajax({type:'HEAD',url:'https://cdn.rawgit.com/jerone/UserScripts/master/PDF_Tools/README.md'}).done(function(data, textStatus, res){console.log(res.status)}).fail(function(res, textStatus, errorThrown){console.log(res.status)});

Of course, we'd have to use rawgit.com for to get around the crossdomain issue.

The problem with http://rawgit.com is that it doesn't guarantee the best uptime. If implemented it would be a none dependable feature, only shown when I correctly returns an usable result, not prohibiting any other actions.

[Martii]

You both are going to love this (sarcasm)

Impossible to vote on a PR that has more than what is issued at #81 so -1 and marking as invalid and assigning a milestone so that @Zren can fix his machine....... and closing. :( aka 😦

As I have stated before we need some of these... make separate issues for items not specific to syncing the *.md ... I am open to separate PRs... @jerone when I said cherry picking for yours that's revoked because @Zren is abusing it here whether intentional or not.

(not going to burn my hands)

You are wise. :)

[Zren]

@Martii

So if I break this up into 8 seperate PRs, you'll merge them all?

All the code is ready to be evaluated, so might as well do it here.

[Martii]

No because #441 exists for you... I created it so we can work on that... I need to know why this is happening and how we can fix it... I went through a similar process with nodejitsu with the git urls.

[Martii]

There's also a potential block from sizzle on your config.js because I already tried this in private... he has his reasons to keep things not in one "big" file... I proposed an app.json to him and he said nay... so we have to persuade him since he pays the bills... the best way to do this is with Issues first then PRs later.

[Zren]

The only reason for the optionalDependency change is so I don't have to mange a second branch for heroku. I can take it out for the merge. Simple enough.

I proposed an app.json

The whole point it to make it a .js file to allow comments and basic logic to fallback on hardcoded defaults if the env variables don't exist. We could break it up into

config.mongoose = require('./config.mongoose');

if having 3 ~8 line files is your thing.

[Martii]

The whole point...

The whole point is that my app.json and your config.js are the same thing just a different language... think the bigger picture... we will have to sell him on this on an issue first. I got him to budge a little but he wants db stuff in db configuration files and so on... basically from my understanding he doesn't want a global configuration. If we start an issue I am sure he'll see it and if not I'll go pound on his proverbial door about it. nodejitsu configuration is also going to be tricky but I still have read access at the moment so I can do some R&D to help make the case.

[Martii]

Reread my posts with any corrections... this is what happens with sleep deprivation when I'm installing a specific distro that took 30 minutes but compile time took 5 retries because I'm unfamiliar with Ubuntu and resulted in 3 hours loss of sleep since that test machine is sooooooo slow.... and I can't comment any more right now ... we'll resume after I rest.

[Zren]

The whole point is that my appSettings.json and your config.js are the same thing just a different language...

A different language that allows abstracting env variables project wide with a bit of logic ala /lib/debug.js? If debug.js wasn't called debug, I'd have actually considered putting them in there.

wants db stuff in db configuration files and so on

See config.mongoose = require('./config.mongoose');

nodejitsu configuration is also going to be tricky but I still have read access at the moment so I can do some R&D to help make the case.

Um... what? There's no change to the env variable names that would need you to change nodejitsu settings.

[Zren]

Now that #441 has been concluded. I'll remove the package.json edits from Zren@811dbfe. I'll be keeping the other part of that change however as (isPro || isDev) == (isPro || !isPro) == true and we need to check for !isDbg too.

So the only things that need to be changed are:

• Undo the optionalDependency change in the package.json. Move the rest of that commit to another PR.
• Undo setting.json refactoring.
  • The config.maximumRequestBodySize = Math.max(config.maximumScriptSize, config.maximumScriptDescriptionSize); will have to move into the app.js. Zren@1b25ff2
  • Moving the AUTH_CALLBACK_URL to the config is put on hold as well since we can't do any logic in JSON. We also lose the code to provide app wide variable for the current hostname without depending on the request headers. Zren@64b1169

While renaming /user/* routes to /account/* could be put in another PR, it will cause Merge Conflicts here. So to not have to deal with them as Martii merges them one by one, it will be part of the same PR.

And all the other commits are actually relevant to the PR. Anything else besides:

• Disable the textarea when sync is checked (also change the color when unfocused to not look like it's disabled).
• Mention the paths are case sensitive.

?

[Martii]

Since the holidays are just around the corner here... off "hold" for everything will probably start sometime next week... not this week. The queue is just about maxed out right now for this upcoming week (don't know about jerones pr but when he comes back from respite to finish that off most likely that should get in this week)... take a break and perhaps reread things especially since I don't care about the filename of config.js vs app.json... that will require sizzles top-level approval anyhow... as I've stated already. :)