Fork improvements #151

izak · 2014-01-14T14:44:33Z

This implements one thing: If you press the fork button while logged in, it will prompt you to log in, and continue with the fork afterwards. It's only step one. What is needed:

You are not logged in. -- Needs to ask you to login in to the account you want to fork the book into.
You already have a fork of this book. -- Needs to say "You already have a copy of this bookshelf. [Take me there] [Cancel]."
Once the fork has occurred, need to poll to see when it is complete, before returning the author to the repo. This change should probably go into octokit.
If possible, the changes that the author made before the fork should be saved back to the new forked copy. If this isn't possible, before doing the fork, need to warn that changes will be lost.

When invoking the logInModal, you can now request that the anonymous option and the extra info be hidden, for when you require a login for a specific change. Use case in mind: Forking while not logged in. The logInModal method now returns a promise that is resolved upon login, or rejected otherwise, so that you can hang a follow-up onto a login.

kathi-fletcher · 2014-01-22T03:58:19Z

OK -- Task 1 looks fine to me!

…pisode1

* Allow forking into an organisation * look for an existing fork * Poll until fork is complete * update url to indicate book switch

The previous version only checked for duplicates in the user area.

…pisode1

izak · 2014-01-28T13:20:34Z

This is as good as it's going to get I think. It depends on a newer octokit, which I have yet to create a PR for. Need to see about updating tests if possible. The functionality to poll the repo wasn't added to octokit. It is simple enough that I decided to keep it inside gh-book.

philschatz · 2014-01-28T13:28:30Z

scripts/gh-book/auth.coffee

+      # If repo exists, go to it or cancel. Else fork.
+      if userinfo.login == login
+        # Forking into own space
+        promise = @model.getClient().getUser().getRepos()


This returns a paginated result. What about calling getClient().getRepo(repoUser, repoName) instead?

That's not going to give me a list of repos for the currently logged in user, or am I reading it wrong?

I want a list of repos to see if an existing fork exists either within the user's own space, or within the organisation he selected for the fork. The else-part on that if does the same thing for organisations, it gets a promise that eventually delivers a list of repos.

Correct; it will not give you a list of repos. It will give you a "repo". I guess I should have suggested .getClient().getRepo(repoUser, repoName).canCollaborate() (or ...getInfo(); basically anything that hits the API)

Oh wait, I see where you're going with this. Instead of asking for a list (which might not contain the name I'm looking for due to pagination), just hit some API call assuming the repo exists, and if it succeeds... then we cannot fork. I agree, getInfo() seems to be the thing I should call.

Ironically this will likely remove the requirement for philschatz/octokit.js#14 :-)

izak · 2014-01-28T14:03:07Z

This presently relies on philschatz/octokit.js#14. I have since noticed that there is a top-level API call in octokit that does what I need without requiring this PR, but I want to hear from Phil if he agrees with my call that having such a method on an organisation makes more sense.

Okay, he's responded on my PR request. Awesome, soon as I have the unit test in place :-)

izak · 2014-01-28T14:30:12Z

Alright, this no longer needs philschatz/octokit.js#14 to work. It does need a newer version of octokit in order to fork into an organisation, so you will have to run npm install again :-(

philschatz · 2014-01-28T14:44:56Z

scripts/gh-book/auth.coffee

+
+        # Poll until repo becomes available
+        pollRepo = () =>
+          @model.getRepo()?.getInfo('').done (info) =>


I do not believe .getInfo() takes an argument, but it's a minor concern ; )

philschatz · 2014-01-28T15:00:35Z

🍰

kathi-fletcher · 2014-01-29T03:36:55Z

I am getting an error running npm install:

[email protected] install /vagrant
bower install

glob error { [Error: EPERM, readdir '/vagrant/bower_components'] errno: 50, code: 'EPERM', path: '/vagrant/bower_components' }
bower EPERM EPERM, readdir '/vagrant/bower_components'

Stack trace:
Error: EPERM, readdir '/vagrant/bower_components'

Console trace:
Trace
at StandardRenderer.error (/vagrant/node_modules/bower/lib/renderers/StandardRenderer.js:69:17)
at Logger.updateNotifier.packageName (/vagrant/node_modules/bower/bin/bower:113:18)
at Logger.EventEmitter.emit (events.js:95:17)
at Logger.emit (/vagrant/node_modules/bower/node_modules/bower-logger/lib/Logger.js:29:39)
at /vagrant/node_modules/bower/lib/commands/install.js:25:16
at _rejected (/vagrant/node_modules/bower/node_modules/q/q.js:808:24)
at /vagrant/node_modules/bower/node_modules/q/q.js:834:30
at Promise.when (/vagrant/node_modules/bower/node_modules/q/q.js:1079:31)
at Promise.promise.promiseDispatch (/vagrant/node_modules/bower/node_modules/q/q.js:752:41)
at /vagrant/node_modules/bower/node_modules/q/q.js:574:44

…pisode1 Conflicts: scripts/gh-book/auth.coffee

kathi-fletcher · 2014-02-04T17:38:41Z

Testing results

You are not logged in. -- Needs to ask you to login in to the account you want to fork the book into.

This seems to work, but when I saved a change to module 1, I got this error "Uncaught NamespaceError: An attempt was made to create or change an object in a way which is incorrect with regard to namespaces. engine.js:73". The change did appear to go through, however.

Something is not working correctly for making changes in the new forked books. If I edit a module I get the error below.
Check here when developer thinks this is fixed.

This seems to work, but when I saved a change to module 1, I got this error "Uncaught NamespaceError: An attempt was made to create or change an object in a way which is incorrect with regard to namespaces. engine.js:73". The change did appear to go through, however.

If you have permission to fork into multiple locations, it should ask you where to fork this.

OK refinements to the testing, now that I have octokit at the correct version. That did not change the results but I have now realized we have a bunch of sub-cases.

Fork into an organization that I have access to.
This worked. I tried wysiwhat and oerpub and the new fork appeared.
Fork into my own repo
Check here when developer thinks this is fixed.

Above is NOT working for me. It does ask, but then after I choose my account (kathi-fletcher), it returns and appears to have done nothing. "Fork This Book" still shows. I get this error in the console "
Uncaught Error: BUG! user argument is required "

You already have a fork of this book. -- Needs to say "You already have a copy of this bookshelf. [Take me there] [Cancel]."
Once the fork has occurred, need to poll to see when it is complete, before returning the author to the repo. This change should probably go into octokit.
Check here when dev thinks this is addressed.

I can't tell if this is working. I think it is. A message flashes during the fork that goes away too fast for me to read. This might be scary to authors, though, so we probably should have some minimum time that the window stays around.

If possible, the changes that the author made before the fork should be saved back to the new forked copy. If this isn't possible, before doing the fork, need to warn that changes will be lost.

OK, this works a little differently than either of those two options, but it is satisfactory. If you make changes and then fork the book, you will have to push the "Save All" button after the fork to save the changes. But the changes will be saved, not lost. So it is a good solution and not too surprising.

izak · 2014-02-06T11:17:33Z

Uncaught Error: BUG! user argument is required " Happens when forking into your own repo instead of an organisation. Reproduced and I will proceed to fix it.

kathi-fletcher · 2014-02-16T19:38:23Z

Alright, I am going to merge this. I have confirmed that the organization and personal account forking work.

This still has a namespace bug when you add a newline to a file. This is important. It probably is some merge problem with the xml changes. I will make a separate ticket.

Fork improvements

izak · 2014-02-17T08:48:00Z

The namespace error happens because Aloha adds a <br> tag when you add a
new paragraph, or at least, this is what I suspect. There is a whole lot of
logic in aloha that deals with this, because when you hit enter you want to
visually see that a new paragraph appears, but some browsers does not show
the extra space if the paragraph is empty, so the <br> is added to force
the effect. I THINK the browser that causes all the trouble is one we don't
care about, an older Internet Explorer, but I'll have to confirm.

On Sun, Feb 16, 2014 at 9:38 PM, Kathi Fletcher [email protected]:

Merged #151 #151.

Reply to this email directly or view it on GitHubhttps://github.com//pull/151
.

izak added 2 commits January 10, 2014 16:18

Prompt for login when forking as anonymous.

d6ed734

izak added 7 commits January 23, 2014 13:35

Merge remote-tracking branch 'origin/master' into fork-improvements-e…

089c542

…pisode1

Use correct terminology: copying the book shelf.

15e89da

Fork improvements.

2c980fb

* Allow forking into an organisation * look for an existing fork * Poll until fork is complete * update url to indicate book switch

Add progress indication modal.

467a306

Check for duplicate repo also in organisation

c6238d4

The previous version only checked for duplicates in the user area.

If there is an existing fork, offer to take me there.

57853cf

Merge remote-tracking branch 'origin/master' into fork-improvements-e…

c62c237

…pisode1

philschatz reviewed Jan 28, 2014
View reviewed changes

Instead of fetching list of repos, call getInfo to see if it is there.

1cb81fe

philschatz reviewed Jan 28, 2014
View reviewed changes

izak added 2 commits January 29, 2014 10:29

getInfo takes no parameter. Cleaning up.

38c0628

Merge remote-tracking branch 'origin/master' into fork-improvements-e…

fb034ec

…pisode1 Conflicts: scripts/gh-book/auth.coffee

Newer octokit needed.

b8c8273

izak added 2 commits February 6, 2014 13:56

Allow forking into personal space

b6b51f9

Delay a bit on closing the modal.

af6fea6

kathi-fletcher added a commit that referenced this pull request Feb 16, 2014

Merge pull request #151 from oerpub/fork-improvements-episode1

1b56098

Fork improvements

kathi-fletcher merged commit 1b56098 into master Feb 16, 2014

kathi-fletcher deleted the fork-improvements-episode1 branch February 16, 2014 19:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fork improvements #151

Fork improvements #151

izak commented Jan 14, 2014

kathi-fletcher commented Jan 22, 2014

izak commented Jan 28, 2014

philschatz Jan 28, 2014

izak Jan 28, 2014

philschatz Jan 28, 2014

izak Jan 28, 2014

izak Jan 28, 2014

izak commented Jan 28, 2014

izak commented Jan 28, 2014

philschatz Jan 28, 2014

philschatz commented Jan 28, 2014

kathi-fletcher commented Jan 29, 2014

kathi-fletcher commented Feb 4, 2014

izak commented Feb 6, 2014

kathi-fletcher commented Feb 16, 2014

izak commented Feb 17, 2014

Fork improvements #151

Fork improvements #151

Conversation

izak commented Jan 14, 2014

kathi-fletcher commented Jan 22, 2014

izak commented Jan 28, 2014

philschatz Jan 28, 2014

Choose a reason for hiding this comment

izak Jan 28, 2014

Choose a reason for hiding this comment

philschatz Jan 28, 2014

Choose a reason for hiding this comment

izak Jan 28, 2014

Choose a reason for hiding this comment

izak Jan 28, 2014

Choose a reason for hiding this comment

izak commented Jan 28, 2014

izak commented Jan 28, 2014

philschatz Jan 28, 2014

Choose a reason for hiding this comment

philschatz commented Jan 28, 2014

kathi-fletcher commented Jan 29, 2014

kathi-fletcher commented Feb 4, 2014

izak commented Feb 6, 2014

kathi-fletcher commented Feb 16, 2014

izak commented Feb 17, 2014