Skip to content
This repository has been archived by the owner on Jul 17, 2020. It is now read-only.

Add common propTypes for Events #398

Open
jrogatis opened this issue Jun 20, 2017 · 24 comments
Open

Add common propTypes for Events #398

jrogatis opened this issue Jun 20, 2017 · 24 comments

Comments

@jrogatis
Copy link
Collaborator

We use a common library of proptypes. Add events type check and change the object at each one of the files that use this prop.

@caskuplich
Copy link
Contributor

@jrogatis I'd like to help with this issue. How can I start?

@gURLmeetsCode
Copy link

@jrogatis I would like to help as well. Please me know if there is anything I can do.

@jrogatis
Copy link
Collaborator Author

jrogatis commented Sep 9, 2017

sorry about the delayed answer! Yes all help will be appreciated! @gURLmeetsCode and @caskuplich ! Its simple. look at the code and find the complex prototypes that repeat along the other component classes and add a file inside at the commom prototypes dir and try to export then there. Any help that you needed please contact me !

@caskuplich
Copy link
Contributor

@jrogatis Let me see if I get it: I found at least 3 files that could use the common propTypes isEvent and isCurUser from the file client/util/commonPropTypes.js:

  • client/components/NavBar/NavBar.js
  • client/components/NotificationBar/NotificationBar.js
  • client/pages/NewEvent/index.js

But I haven't identified patterns of complex propTypes that could be reused by components. Maybe you can help me find these propTypes.

@jrogatis
Copy link
Collaborator Author

@caskuplich
curUser: PropTypes.shape({
_id: PropTypes.string, // Unique user id
name: PropTypes.string, // User name
avatar: PropTypes.string, // URL to image representing user(?)
}).isRequired,

is one of than is used all over the app.

@caskuplich
Copy link
Contributor

@jrogatis Unfortunately I can't run the project on my local machine. After Google redirects to the app, the following error appears:

Error
    at /<...>/meeting-for-good/node_modules/passport-google-oauth20/lib/strategy.js:95:21
    at passBackControl (/<...>/meeting-for-good/node_modules/oauth/lib/oauth2.js:132:9)
    at IncomingMessage.<anonymous> (/<...>/meeting-for-good/node_modules/oauth/lib/oauth2.js:157:7)
    at emitNone (events.js:110:20)
    at IncomingMessage.emit (events.js:207:7)
    at IncomingMessage.wrappedEmit [as emit] (/<...>/meeting-for-good/node_modules/opbeat/lib/instrumentation/http-shared.js:116:29)
    at endReadableNT (_stream_readable.js:1045:12)
    at instrumented (/<...>/meeting-for-good/node_modules/opbeat/lib/instrumentation/index.js:102:27)
    at _combinedTickCallback (internal/process/next_tick.js:138:11)
    at process._tickDomainCallback (internal/process/next_tick.js:218:9)
    at process.fallback (/<...>/meeting-for-good/node_modules/opbeat/lib/instrumentation/async-hooks.js:514:17)

Node version: v8.3.0
MongoDB version: v3.4.7
OS: Trisquel 7 (Ubuntu 14.04)

Can you help me with this issue?

@jrogatis
Copy link
Collaborator Author

@caskuplich Hey there. Sure! Do you set the .env file properly with the correct api keys ?

@caskuplich
Copy link
Contributor

@jrogatis I just didn't set the FACEBOOK_KEY, FACEBOOK_SECRET, AWS_ACCESS_KEY_ID, AWS_SECRET_KEY, and EMAIL_FROM.

Is there any problem to set the URL restrictions and redirect_url to http://localhost:8080 and http://localhost:8080/api/auth/google/callback, respectively on the Google API Console when I create a new OAuth API client_id? I enabled the Calendar API as stated on the README file and also created an API Key for Google Analytics.

@jrogatis
Copy link
Collaborator Author

jrogatis commented Sep 13, 2017

You need to set this call back properly at the google api console.
http://localhost:8080/api/auth/google/callback
at the javascript authorized URLs at restrictions area.
@caskuplich

@caskuplich
Copy link
Contributor

This is my Google Developers console settings:

console

This is a OAuth Client ID credential.

It seems that the problem is on the meeting-for-good code on the return. For some reason the code returned from Google causes the error I mentioned above. The URL of the return comes with a code query string, like this: http://localhost:8080/api/auth/google/callback?code=<code with 46 chars>. This is right, isn't it?

@jrogatis
Copy link
Collaborator Author

do you set the Oauth consentiment screen ? and enable the API ? @caskuplich ?

@caskuplich
Copy link
Contributor

@jrogatis, yes. I set my email address and the Product Name as "Meeting for Good Dev". The other fields are optional.

Can I sign-in the application with the same email address I set in the Consent screen or should I enter with a different Google Account?

@caskuplich
Copy link
Contributor

I'm suspicious that it's a version problem. I think that there is some breaking change in passport.js or other related package. @jrogatis, can you make a new clone of the repo run on your local machine?

@jrogatis
Copy link
Collaborator Author

@caskuplich I will try today.

@jrogatis
Copy link
Collaborator Author

@caskuplich its running perfectly.

@caskuplich
Copy link
Contributor

@jrogatis, thank you so much. I'll try to find what is wrong with my local machine setup.

@jrogatis
Copy link
Collaborator Author

what . errors you got at the back ? @caskuplich ?

@caskuplich
Copy link
Contributor

caskuplich commented Sep 19, 2017

The same errors I posted above, @jrogatis. What makes this hard to solve is that there isn't a good error message, unfortunately.

@caskuplich
Copy link
Contributor

@jrogatis Yeah! Now I can login! I had to enable the Google+ API to make this work. So, my Google API console dashboard has the Google Calendar API, Google+ API and the Analytics API enabled. Thank you again @jrogatis. Now I can work on this issue on my local machine. 😄

@jrogatis
Copy link
Collaborator Author

great @caskuplich !

@jrogatis jrogatis reopened this Sep 19, 2017
@caskuplich
Copy link
Contributor

@jrogatis , I DRYed the NotificationBar component (client/components/NotificationBar/NotificationBar.js) with no problems. But I tried to change the NavBar component (client/components/NavBar/NavBar.js) and it issues a warning on the web console saying that _id and avatar are required when the user isn't logged in. The current version doesn't issue any warning regarding propTypes. Can you help me with this?

I'd also want to DRY the client/pages/NewEvent/index.js file, and I noted that it is already issuing a warning on the console about the user propType right now (current version). I think that we should fix this warning before DRYing this file. What do you think?

@jrogatis
Copy link
Collaborator Author

@caskuplich can you give me your branch url ? so i can see the code and help with ?

@caskuplich
Copy link
Contributor

@jrogatis Here is my branch URL: https://github.com/caskuplich/meeting-for-good/tree/fix/refactor-proptypes

I think the NotificationBar can be merged with no problems, but the NavBar issues propTypes warnings saying that _id, name, and avatar from curUser are undefined even when the user is logged in.

I also noticed that the client/pages/NewEvent/index.js (current version, I didn't modify it yet) issues a warning saying curUser propType is undefined just the first time we click the + button at the bottom right corner of the screen. When we click the next times it doesn't issue any warning.

@jrogatis
Copy link
Collaborator Author

i will adress this ASAP @caskuplich for some reason I miss your last comment

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants