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

fix(learn): advanced-node-and-express username field #47349

Conversation

Xavier-Pierre-dev
Copy link
Contributor

Checklist:

  • I have read freeCodeCamp's contribution guidelines.
  • My pull request has a descriptive title (not a vague title like Update index.md)
  • My pull request targets the main branch of freeCodeCamp.
  • I have tested these changes either locally on my machine, or GitPod.

Closes #XXXXX

I notice that for the moment the curriculum have a little coherency issue starting at github authentication. In reality at the start of the project we set up our strategy with the username property for example when we register an user using the form the curriculum will push us to use :

app.route('/register')
  .post((req, res, next) => {
    myDataBase.findOne({ username: req.body.username }, function(err, user) {
      if (err) {
        next(err);
      } else if (user) {
        res.redirect('/');
      } else {
        myDataBase.insertOne({
          username: req.body.username,
          password: req.body.password
        },
          (err, doc) => {
            if (err) {
              res.redirect('/');
            } else {
              // The inserted document is held within
              // the ops property of the doc
              next(null, doc.ops[0]);
            }
          }
        )
      }
    })
  },
    passport.authenticate('local', { failureRedirect: '/' }),
    (req, res, next) => {
      res.redirect('/profile');
    }
  );

As you can see we use username: req.body.username then we also use that to display content on profile.pug. In reality username property are use since start until the github authentication. In fact for github authentication username property are not set up so later with socket we use name instead of username this lead to an issue where if we login with something else than github the name will be undefined for the chat app and not be displayed.

This is why I propose some change inside the curriculum in order to add the username property when a user use github in order to register himself and edit the next step so the challenge guideline will push the learner to use the username instead of the user. To make it properly work either it's with simple register form, login with or without github

@gitpod-io
Copy link

gitpod-io bot commented Aug 22, 2022

@github-actions github-actions bot added the scope: curriculum Lessons, Challenges, Projects and other Curricular Content in curriculum directory. label Aug 22, 2022
@ghost
Copy link

ghost commented Aug 22, 2022

👀 Review this PR in a CodeSee Review Map

View the CodeSee Map of this change

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map Legend

@raisedadead raisedadead added the status: waiting review To be applied to PR's that are ready for QA, especially when additional review is pending. label Aug 22, 2022
@raisedadead raisedadead changed the title challenges: quality assurance update .md files to keep consistency using username fix(learn): quality assurance - keep consistency using username Aug 22, 2022
@Xavier-Pierre-dev Xavier-Pierre-dev requested review from hanswang123456 and removed request for ShaunSHamilton August 23, 2022 17:47
shorter the sentence as proposed by hanswang123456
@Xavier-Pierre-dev
Copy link
Contributor Author

Sorry I make a mistake with that :
Xavier-Pierre-dev requested review from hanswang123456 and removed request for ShaunSHamilton 8 minutes ago

It wasn't intentional, I clicked by mistake, this is my first PR.

Copy link
Contributor

@hanswang123456 hanswang123456 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks Good!

@hanswang123456
Copy link
Contributor

@Xavier-Pierre-dev Please get @ShaunSHamilton, @Sboonny, or @moT01 to review as they are actual reviewers. I'm just looking out for any small fixes.

@hanswang123456
Copy link
Contributor

Also check out issue #47109. Might be a nice issue for you to attempt.

@ShaunSHamilton ShaunSHamilton self-assigned this Aug 23, 2022
Copy link
Member

@ShaunSHamilton ShaunSHamilton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello there,

I am requesting changes to this PR to block it for now, because this will require the relevant Gists to be updated as well - something a member of staff will have to do.

@Sembauke Sembauke added status: blocked Is waiting on followup from either the Opening Poster of the issue or PR, or a maintainer. and removed status: waiting review To be applied to PR's that are ready for QA, especially when additional review is pending. labels Sep 9, 2022
@raisedadead raisedadead added status: blocked Is waiting on followup from either the Opening Poster of the issue or PR, or a maintainer. status: PR in works Work in Progress (WIP) Issues. and removed status: blocked Is waiting on followup from either the Opening Poster of the issue or PR, or a maintainer. labels Oct 7, 2022
@raisedadead
Copy link
Member

@ShaunSHamilton let's review this and do any additional tasks. This has been sitting here for a while.

@ShaunSHamilton
Copy link
Member

To mention, this is related to: #39599

@ShaunSHamilton
Copy link
Member

ShaunSHamilton commented Oct 7, 2022

This is ready to go in. Once we are production ready, we need to have the Gists updated based off of mine here: https://gist.github.com/ShaunSHamilton

Also, the boilerplate needs updating: <insert_pr_once_made>

@ShaunSHamilton ShaunSHamilton removed the status: PR in works Work in Progress (WIP) Issues. label Oct 7, 2022
Co-authored-by: Kristofer Koishigawa <scissorsneedfoodtoo@gmail.com>
@moT01 moT01 added status: blocked Is waiting on followup from either the Opening Poster of the issue or PR, or a maintainer. and removed status: waiting review To be applied to PR's that are ready for QA, especially when additional review is pending. labels Oct 14, 2022
Copy link
Member

@moT01 moT01 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This all looks real good @Xavier-Pierre-dev 🎉

I went through the whole thing pretty thoroughly. The project is pretty awesome - I never actually went through it like that.

I made quite a few suggestions to try and improve the instructions. Some of them were typos or other grammar suggestions in your additions, but most were because I was having a hard time differentiating explanation from instructions of what I was supposed to do. So it's mostly an attempt to clarify to campers what they are actually supposed to do or add - which I think is overdue. Also, the instructions for creating an app on GH needed some updating. Many of these were pre-existing and possibly outside the scope of this PR, but I think we should add them here. Let me know if you have any questions on any of it.

Co-authored-by: Tom <20648924+moT01@users.noreply.github.com>
Copy link
Contributor

@scissorsneedfoodtoo scissorsneedfoodtoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was only able to get through about half of the project, but everything is looking much better than before.

A lot of the things I noticed are pretty minor, so I'd be fine with disregarding a lot of them. Just wanted to point them out since there are a lot of other improvements here.

ShaunSHamilton and others added 2 commits October 17, 2022 14:31
Co-authored-by: Kristofer Koishigawa <scissorsneedfoodtoo@gmail.com>
Co-authored-by: Kristofer Koishigawa <scissorsneedfoodtoo@gmail.com>
moT01
moT01 previously approved these changes Oct 18, 2022
Copy link
Member

@moT01 moT01 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM @ShaunSHamilton 🎉

Copy link
Contributor

@scissorsneedfoodtoo scissorsneedfoodtoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for all of your hard work on this @Xavier-Pierre-dev, and all the other improvements @ShaunSHamilton and @moT01 made look great. The instructions are much easier to follow now.

I went through the rest of the project and everything LGTM 👍

@naomi-lgbt
Copy link
Member

Heya @moT01 is this still blocked? Or can we go ahead and get this in?

@raisedadead
Copy link
Member

can we go ahead and get this in?

I believe as soon as the helper solutions are ready on the guide we are good to go.

@ShaunSHamilton
Copy link
Member

Are we at the point I should add the solutions to this PR?

Sboonny
Sboonny previously approved these changes Oct 31, 2022
@Sboonny Sboonny dismissed their stale review October 31, 2022 12:48

Approved by mistake 😓

@naomi-lgbt
Copy link
Member

I think this is actually blocked until freeCodeCamp/boilerplate-advancednode#25 lands?

@moT01 moT01 added status: waiting review To be applied to PR's that are ready for QA, especially when additional review is pending. and removed status: blocked Is waiting on followup from either the Opening Poster of the issue or PR, or a maintainer. labels Nov 1, 2022
@moT01
Copy link
Member

moT01 commented Nov 1, 2022

No, this needs to get in first, and once it hits production - we can merge that PR @naomi-lgbt.

Copy link
Member

@naomi-lgbt naomi-lgbt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, this looks good. The tests all pass with Shaun's solutions (and the updated boilerplate), and I don't see any issues with the copy.

@raisedadead raisedadead merged commit de02417 into freeCodeCamp:main Nov 1, 2022
@Xavier-Pierre-dev
Copy link
Contributor Author

Sorry I wasn't able to see all of the avancement on this PR before today.

I also wanted to thanks @moT01, @ShaunSHamilton, @scissorsneedfoodtoo and all the contributors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope: curriculum Lessons, Challenges, Projects and other Curricular Content in curriculum directory. status: waiting review To be applied to PR's that are ready for QA, especially when additional review is pending.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants