feat: Finish implementing project creation on backend (#21) #38

ermshiperete · 2019-12-02T15:19:29Z

This change finishes the implementation of creating a project on the backend (#21).

This change also implements extracting the keyboard id from the keyboard_info file (part of #10).

This change is

This change finishes the implementation of creating a project on the backend.

This implements parts of keymanapp#10.

mcdurdin

I'd like to see some "why" or "reason for existence" comments on some of the functions because there's a lot of code and a bit of context can really help understanding :)

It might be good to factor out all the URL strings embedded into a single location.

mcdurdin · 2019-12-05T13:40:03Z

README.md

@@ -32,6 +32,15 @@ In the `*.env` file replace the values for `CLIENT_ID` and `CLIENT_SECRET` with
 _Client Secret_ that GitHub displays for the app. You should also replace the value for
 `SESSION_SECRET` with a random value.

+If you want to run all e2e tests, you'll have to create a test user on GitHub. Fork
+[kdotester1/khmer_angkor](https://github.com/kdotester1/khmer_angkor) to your test


Perhaps we should create a repo on @keymanapp with a good 'testing' name such as "test_kdo_khmer_angkor" and use that rather than relying on another repo that may run into maintenance and access issues in the future?

Good idea. Can you fork kdotester1/test_kdo_khmer_angkor and keymanapptest/test_kdo_keyboards into @keymanapp?

mcdurdin · 2019-12-05T14:05:32Z

test/projects.e2e-spec.ts

+  let githubService: GithubService;
+
+  async function deleteKeyboardsRepo(): Promise<void> {
+    await httpService.delete(`https://api.github.com/repos/${user}/keyboards`, {


This seems like a dangerous test. Do we need to reinforce that this test must not be run with the developer's real GH account?

With the keyboards repo renamed test_kdo_keyboards this should be save now.

- rename repos used in e2e tests - add some comments - some cleanup of unused code

ermshiperete · 2019-12-06T14:45:21Z

I'd like to see some "why" or "reason for existence" comments on some of the functions because there's a lot of code and a bit of context can really help understanding :)

Can you point out the methods where this would be good?

mcdurdin · 2019-12-07T11:32:44Z

Can you point out the methods where this would be good?

For example, waitForRepoToExist. I can see what the code is doing but some context would be helpful. I like to see a one line description of the function because when someone else comes into the code, the 'why' is the hardest part. The code you've written is generally clear and easy to understand so there's no issue with the implementation (although I have to ask if it is right that waitForRepoToExist function really could ping GitHub 300 times in 5 minutes? That could cause trouble with rate limiting? Perhaps back-off might be advisable...)

mcdurdin · 2019-12-07T12:38:16Z

Okay, I've done the review but currently having trouble getting the test environment to run on my machine. Not sure if I am missing something in the setup (noting also that README.md between the root and frontend folders are perhaps slightly inconsistent)

mcdurdin · 2019-12-08T09:27:29Z

Okay, got the dev environment running. It seems to be mostly working, so far.

Testing notes

Workflow issue if forking repo late

I forked the repo after loading the KDO home page. This led to an issue where KDO did not show my keyboard. Instead, I was shown a spinner with a link "I don't see my keyboard project".

I then forked the repo:

and clicked the link. This led to a blank screen (just the top banner visible):

The second time I tried to repro this, I got the following:

This may correspond to a warning in the log:

(node:17644) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 disconnect listeners added to [MemoryStore]. Use emitter.setMaxListeners() to increase limit
(node:17644) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 connect listeners added to [MemoryStore]. Use emitter.setMaxListeners() to increase limit

So refreshing the page didn't help; I had to logout and login again, to see the repo:

Once we get here, I click on the link and am presented with a blank screen but then I think that's as far as it goes in this PR, right?

UX

It may be a little early to be worrying about this but the grid layout with small boxes for repo names (see above) are a little awkward. May be better to use a list layout for this.

ermshiperete · 2019-12-11T10:07:12Z

Once we get here, I click on the link and am presented with a blank screen but then I think that's as far as it goes in this PR, right?

yes, this PR only changes things in the backend, so everything you experienced is expected (with the exception of the "Too many requests" (#41)).

It may be a little early to be worrying about this but the grid layout with small boxes for repo names (see above) are a little awkward. May be better to use a list layout for this.

I created #40 for that.

ermshiperete added 4 commits December 2, 2019 16:17

feat: Finish implementing project creation on backend (keymanapp#21)

7f6265f

This change finishes the implementation of creating a project on the backend.

feat: Extract keyboard id from keyboard_info or repo name

5575e42

This implements parts of keymanapp#10.

feat: Use extracted keyboard id as return value for createRepo

d43e99b

docs: Update readme for e2e tests

5c294b6

darcywong00 added this to the P8S5 milestone Dec 4, 2019

ermshiperete added the enhancement New feature or request label Dec 4, 2019

ermshiperete self-assigned this Dec 4, 2019

mcdurdin requested changes Dec 5, 2019

View reviewed changes

Follow-up of code review

a1705e7

- rename repos used in e2e tests - add some comments - some cleanup of unused code

docs: Add comments on some methods

cb79b4f

mcdurdin approved these changes Dec 11, 2019

View reviewed changes

mcdurdin modified the milestones: P8S5, P8S6 Dec 14, 2019

mcdurdin modified the milestones: P8S6, P8S7 Jan 1, 2020

ermshiperete merged commit 5388e29 into keymanapp:master Jan 6, 2020

ermshiperete deleted the gitrepo branch January 6, 2020 09:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Finish implementing project creation on backend (#21) #38

feat: Finish implementing project creation on backend (#21) #38

ermshiperete commented Dec 2, 2019 •

edited

Loading

mcdurdin left a comment

mcdurdin Dec 5, 2019

ermshiperete Dec 6, 2019

mcdurdin Dec 5, 2019

ermshiperete Dec 6, 2019

ermshiperete commented Dec 6, 2019

mcdurdin commented Dec 7, 2019

mcdurdin commented Dec 7, 2019

mcdurdin commented Dec 8, 2019 •

edited

Loading

ermshiperete commented Dec 11, 2019

feat: Finish implementing project creation on backend (#21) #38

feat: Finish implementing project creation on backend (#21) #38

Conversation

ermshiperete commented Dec 2, 2019 • edited Loading

mcdurdin left a comment

Choose a reason for hiding this comment

mcdurdin Dec 5, 2019

Choose a reason for hiding this comment

ermshiperete Dec 6, 2019

Choose a reason for hiding this comment

mcdurdin Dec 5, 2019

Choose a reason for hiding this comment

ermshiperete Dec 6, 2019

Choose a reason for hiding this comment

ermshiperete commented Dec 6, 2019

mcdurdin commented Dec 7, 2019

mcdurdin commented Dec 7, 2019

mcdurdin commented Dec 8, 2019 • edited Loading

Testing notes

Workflow issue if forking repo late

UX

ermshiperete commented Dec 11, 2019

ermshiperete commented Dec 2, 2019 •

edited

Loading

mcdurdin commented Dec 8, 2019 •

edited

Loading