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: JS null error linked to newsletter form #962

Merged
merged 5 commits into from
Jun 7, 2019
Merged

Conversation

faucomte97
Copy link
Contributor

@faucomte97 faucomte97 commented Jun 7, 2019

This fixes the poltergeist bug!

Initial problem

The newsletter form included a hidden label which was populated with JS when the website loaded. It was populated with the URL of the page the user was currently on. Since this hidden label was in the form, it was included in the POST request and the Django form then used the URL from the POST data to be able to redirect the user to the same page upon submission of the form. This was most likely because the newsletter form is in the footer (therefore, the base HTML template) and it wasn't straightforward to know how to redirect the user to the same page. More details can be found about how this was implemented by looking at PR #716.
The was the JS populated the hidden label sometimes caused a null error. We think that this happened sometimes depending on loading speed, or maybe overall traffic on the website. In general, it seemed like if or when the webpage loaded slowly, the HTML element hadn't loaded properly and the JS wasn't able to find it, causing a null error.
Because the JS raised an error at that point in the {% scripts %} block, none of the following JS could be executed. This included the other JS files such as the level selection logic (explains why the white squares would show next to the episode names), the scoreboard logic (explains why the scoreboard would load without any borders, background colours, etc...) and the level moderation logic (explains why the student dropdown list would not load, populate or work properly).

Solution

This PR makes the newsletter form redirect automatically to the home page, instead of the current page the user was on. This means the faulty JS and the hidden label could be removed.
This is also a fix that could later be further improved by actually figuring out how to properly redirect to the same page in Django, however this proved to be more challenging than expected, and the urgency of fixing this production bug pushed me towards this solution.

@faucomte97 faucomte97 self-assigned this Jun 7, 2019
@mrniket
Copy link
Contributor

mrniket commented Jun 7, 2019

This change is Reviewable

@faucomte97 faucomte97 changed the title Fix JS null error linked to newsletter form fix: JS null error linked to newsletter form Jun 7, 2019
Copy link
Contributor

@MMFernando MMFernando left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r1, 2 of 2 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @faucomte97)

Copy link
Contributor

@MMFernando MMFernando left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @faucomte97)

@faucomte97 faucomte97 merged commit 44e1210 into master Jun 7, 2019
@faucomte97 faucomte97 deleted the fix-poltergeist branch June 7, 2019 12:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants