Wiredcraft / test-fullstack

6 stars 43 forks source link

Task for ChopperLee2011 #1

Closed fraserxu closed 9 years ago

fraserxu commented 9 years ago

Task

Build a simple application to do oauth with Github api, use the token to load repos and issues from Github and display it in the client.

Mockup

mock

Requirement

  1. [Login page] The user could use Github to do oauth(a "login with Github" button on the login page);
    • router /login
    • use express/koa in the backend to do oauth with Github and save a new user in the database(couchdb/mongodb/redis);
  2. [Repos list page] After user login success, go to the repo list page;
    • router /repos
    • use the access_token to get the list of repos from Github(this could be done from the back-end or you can use the token in the client to get the repo list), and then display them in the /repos page;
  3. [Issues list page] After User click the repo name, go to /repos/repo/:reponame page, get the list of issues;
    • router /repo/:repoName
    • user could see all the issues from the repo
  4. [Issue detail page] User could click the issue name and see the detail of the issues;
    • router /repo/:repoName/:issueNumber
    • display the issue title, and issue body(render markdown content to html)

      Goal

    • Knowledge with Oauth
    • Use 3rd party API
    • Github workflow
    • "MEAN" stack. But I would be happy to see new way to do it, like couchdb + koa + reactjs + nodejs Of course, you can still use the real MEAN stack.

If you have any question, please don't hesitate to ask me here.

ChopperLee2011 commented 9 years ago

Hi fraserxu, Sorry, cause i work overtime last Saturday, so not finish the task yet. My current work still have some problem mostly session control. Could i still have time to try to finish it this evening.And the deadline is?

fraserxu commented 9 years ago

@ChopperLee2011 No problem.

We usually don't have a clear deadline of our tasks. You can finish it when you have time. Don't worry about the time. :D

ChopperLee2011 commented 9 years ago

@fraserxu Hi, i think my code is ready for review, simple to achieve functional.

quentinberder commented 9 years ago

@ChopperLee2011 where did you push your code?

ChopperLee2011 commented 9 years ago

@quentinberder @fraserxu Sorry, forgot to add the address: github-oauth-repo

fraserxu commented 9 years ago

Good job @ChopperLee2011 I've seen you have made some good progress!

I just checked the code and find a few issues(mainly client side):

  1. xmlhttprequest and underscore are not listed(but required for running) in the package.json(I guess it because of the issue you mention in the readme about the performance?);
  2. I saw $locationProvider.html5Mode(true) in your code, but not sure if you notice that the app will not work if user refresh the page, so we usually do not use this(using # instead).
  3. When open a page that need to load async data, you have a placeholder like no data in the page, which may leave the user the expression that there's no data, but the data is actually loading, so I suggest you to change that.
  4. The content of the issue body is parsed with markdown but not render properly in the page. You can check the $sanitize service to fix it.
  5. The Gravatar is not loaded.
xeodou commented 9 years ago

Backend

  1. https://github.com/ChopperLee2011/github-oauth-repo/blob/master/routes.js#L9 express has res.render functon, maybe you can have a look. http://expressjs.com/4x/api.html#res.render 2.https://github.com/jaredhanson/passport/blob/298d2452430dd0d1841d417e9c0d0b23e4b06239/lib/http/request.js#L72 Passport is already clean the session you don't need destory session here https://github.com/ChopperLee2011/github-oauth-repo/blob/master/routes.js#L17 anymore
  2. Maybe you can put https://github.com/ChopperLee2011/github-oauth-repo/tree/master/api/auth/github-api this in package.json ,then use it as a module will be better
  3. I don't really think use jwt to create a cookie token is a good idea, passport and express session can do it very well.
makara commented 9 years ago

@xeodou

you don't need destory session here

You don't need to but you can; destroying it can be a good thing if you have security concerns.

@ChopperLee2011

Good work so far. Focus on the frontend if you don't plan to dig into backend too much.

ChopperLee2011 commented 9 years ago

@fraserxu thanks for your advice, and my answer is:

  1. Yes, i change a little about github-api, and want to make it more suitable for this task.So i pick it out of the public/lib directory, if i have time i will try to rewrite it.
  2. (`・ω・´) i misunderstand the task desc, then add the html5Mode(true) to match the router without ‘#’, and do not consider the bug. 3&4 thanks for your suggestions.
  3. Avatars loaded well at my last test, so i will check it this evening.

i will read last replies careful this evening after work, thanks guys.

ChopperLee2011 commented 9 years ago

@xeodou thanks, i just feel tension that you review my codes so carefully~~ and here is my replies for your feedback

  1. render vs sendfile, view engine vs static file. cause i use HTML file, not ejs or jade, so i do not use render at first.
  2. cause there are still some session values after passport's logout, so i destroy the session for safe.
  3. cause i want to expand the github-api lib for some reason, so just don't know how to handle it in package.json.
  4. thanks for your suggestion, i will check this, maybe you are right. @makara OK, i will start to improve the frontend.
ChopperLee2011 commented 9 years ago

@fraserxu i create a new branch: meanjs, and upload the code, but it still upload node_module even i have the gitignore file, i do not know why?

xeodou commented 9 years ago

Because the node_module is already in your git trees. You need use git rm -rf node_modules rm the folder from git trees.

fraserxu commented 9 years ago

@xeodou :+1:

ChopperLee2011 commented 9 years ago

@xeodou Thanks

hunvreus commented 9 years ago