-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
feat: Integrate web app generator with event dashboard #5529
feat: Integrate web app generator with event dashboard #5529
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/eventyay/open-event-frontend/8mdkb0648 |
817e36a
to
18b233d
Compare
18b233d
to
53ecdf4
Compare
Codecov Report
@@ Coverage Diff @@
## development #5529 +/- ##
===============================================
+ Coverage 23.07% 23.71% +0.63%
===============================================
Files 494 498 +4
Lines 5210 5255 +45
Branches 38 44 +6
===============================================
+ Hits 1202 1246 +44
Misses 4003 4003
- Partials 5 6 +1
Continue to review full report at Codecov.
|
- Earlier generate web app button was not working - Redirect to web app generator in a new tab and fill and submit form automatically by fetching details from the event dashboard - Create redirection web app generator url, pass it to views for button to work - Include the redirection host for development and production environment in environment config file - Update readme of local setup
53ecdf4
to
001ed94
Compare
@iamareebjamal can you please review this PR? |
export default class EventApps extends Component { | ||
@computed('eventId') | ||
get webAppGeneratorUrl() { | ||
if (this.authManager.currentUser) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for this check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@iamareebjamal, otherwise one test will fail in travis ci as it doesn't sign in to test this feature. I included this only after I found that failing case.
Travis CI build - https://travis-ci.com/github/fossasia/open-event-frontend/builds/199132180
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to set the email of the user in the test. Not checking the case is even worse. It may pass the test and break in production
if (this.authManager.currentUser) { | ||
return `${ENV.appGenerator.webHost}/?email=${this.authManager.currentUser.email}&apiendpoint=${ENV.APP.apiHost}/${ENV.APP.apiNamespace}/events/${this.eventId}`; | ||
} | ||
return '#'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would you return a
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall we return ${ENV.appGenerator.webHost}/
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the check is removed, it won't be needed
export default class EventApps extends Component {} | ||
export default class EventApps extends Component { | ||
@computed('eventId') | ||
get webAppGeneratorUrl() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indention error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@iamareebjamal I think it is fine because it got automatically indented like this when I did git commit. Let me know how many spaces need to be included.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2
@@ -7,7 +7,7 @@ | |||
<button class="ui button">{{t 'Generate Android App'}}</button> | |||
</div> | |||
<div class="column eight wide center aligned"> | |||
<button class="ui button">{{t 'Generate Web App'}}</button> | |||
<a href="{{this.webAppGeneratorUrl}}" target="_blank" class="ui button" rel="noopener">{{t 'Generate Web App'}}</a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<a href="{{this.webAppGeneratorUrl}}" target="_blank" class="ui button" rel="noopener">{{t 'Generate Web App'}}</a> | |
<a href="{{this.webAppGeneratorUrl}}" target="_blank" class="ui button" rel="noopener noreferrer">{{t 'Generate Web App'}}</a> |
config/environment.js
Outdated
torii: {} | ||
torii: {}, | ||
|
||
appGenerator: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not appGenerator, it's webAppGenerator
config/environment.js
Outdated
torii: {}, | ||
|
||
appGenerator: { | ||
webHost: process.env.WEB_APP_GENERATOR_HOST || (environment === 'production' ? 'https://open-event-wsgen.herokuapp.com' : 'https://open-event-wsgen-dev.herokuapp.com') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for a nested field, just write it as webAppGeneratorUrl
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made it nested because we have another button for the android app generator as well. Let me know if I need to make it flat. @iamareebjamal
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That will be another field
docs/installation/local.md
Outdated
@@ -34,6 +34,8 @@ brew link --force gettext | |||
``` | |||
|
|||
- By default, the `.env.example` file specifies the `API_HOST` as `https://open-event-api-dev.herokuapp.com` which is a test deployment of the open-event-server. If you intend to work on just the frontend, this is sufficient. **If however, you intend to work on issues which involve both the frontend and the backend, you must have the [open-event-server](https://github.com/fossasia/open-event-server) already up and running. Please install and set it up first before changing the URL for `API_HOST` to `http://localhost:5000` and proceeding to run the frontend.** | |||
|
|||
- By default, the `.env.example` file specifies the `WEB_APP_GENERATOR_HOST` as `https://open-event-wsgen-dev.herokuapp.com` which is a test deployment of the open-event-wsgen. If you intend to work on just the frontend, this is sufficient. **If however, you intend to work on issues which involve both the frontend and the website generator, you must have the [open-event-wsgen](https://github.com/fossasia/open-event-wsgen) already up and running. Please install and set it up first before changing the URL for `WEB_APP_GENERATOR_HOST` to `http://localhost:5000` and proceeding to run the frontend.** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
localhost:5000 is server URL, it'll conflict
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do I need to change the port for wsgen? Shall I change to 3000? Let me know I'll include in the current PR for this feature in the wsgen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
.env.example
Outdated
@@ -2,3 +2,4 @@ GOOGLE_API_KEY="Sample Key" | |||
API_HOST=https://open-event-api-dev.herokuapp.com | |||
MAP_DISPLAY=embed | |||
FASTBOOT_DISABLED=true | |||
WEB_APP_GENERATOR_HOST=https://open-event-wsgen-dev.herokuapp.com |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the default one is already specified in env and this is not a major component, please remove it from here
.env.example
Outdated
FASTBOOT_DISABLED=true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FASTBOOT_DISABLED=true | |
FASTBOOT_DISABLED=true | |
@@ -7,7 +7,7 @@ | |||
<button class="ui button">{{t 'Generate Android App'}}</button> | |||
</div> | |||
<div class="column eight wide center aligned"> | |||
<a href="{{this.webAppGeneratorUrl}}" target="_blank" class="ui button" rel="noopener">{{t 'Generate Web App'}}</a> | |||
<a href="{{this.webAppGeneratorUrl}}" target="_blank" class="ui button" rel="noreferrer">{{t 'Generate Web App'}}</a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<a href="{{this.webAppGeneratorUrl}}" target="_blank" class="ui button" rel="noreferrer">{{t 'Generate Web App'}}</a> | |
<a href="{{this.webAppGeneratorUrl}}" target="_blank" class="ui button" rel="noopener noreferrer">{{t 'Generate Web App'}}</a> |
…nerator env variable flat instead of nested, replace noopener with noreferrer in rel, update local docs for changes in env variables, remove webAppGenerator from env.example as it is not a major integration
4d6886b
to
b3794b8
Compare
…readme to use port 3000 for wsgen
d159d76
to
17ae6d1
Compare
Fixes #5476
Short description of what this resolves:
Changes proposed in this pull request:
Checklist
development
branch.