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.

Sign up for GitHub

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/infinite loop #1234

Merged
merged 8 commits into from
May 24, 2021
Merged

Fix/infinite loop #1234

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
8987ddd
Prevent infinity loop when listingId is not defined
dominikx96 May 12, 2021
d5b0b07
Override language from the context
dominikx96 May 12, 2021
cf68ede
Updated test
dominikx96 May 12, 2021
d1a729a
Redirect to the homepage when there is no listingId
dominikx96 May 17, 2021
a33fade
Updated cypress test
dominikx96 May 17, 2021
5697ea7
Updated autofill to do not override selected language
dominikx96 May 20, 2021
063bf15
Merge branch 'master' into fix/infinity-loop
dominikx96 May 20, 2021
dcb8998
Cleanup
dominikx96 May 21, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion sites/public/cypress/integration/pages/ApplicationForms.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
describe("New applications page", function () {
it("renders the new application form", function () {
cy.visit("/applications/start/choose-language")
cy.get("form").should("have.length.of.at.least", 1)
cy.get("h3.field-label--caps").contains("Choose Your Language")
})
})
8 changes: 6 additions & 2 deletions sites/public/lib/appAutofill.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,13 @@ import { blankApplication } from "@bloom-housing/ui-components"

class AutofillCleaner {
application: Application = null
contextApplication: Application = null

constructor(application: Application) {
// provide context value to override current application language choosen by user on the first step
// eslint-disable-next-line @typescript-eslint/no-explicit-any
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see anything untyped here…do we still need the disable comment?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

nice catch! Removing it...

constructor(application: Application, contextApplication: any) {
this.application = application
this.contextApplication = contextApplication
}

clean() {
Expand All @@ -33,10 +37,10 @@ class AutofillCleaner {
this.application["completedSections"] = 0 // only used on frontend
this.application["autofilled"] = true // only used on frontend
this.application.submissionType = ApplicationSubmissionType.electronical
this.application.language = Language.en
this.application.acceptedTerms = false
this.application.status = ApplicationStatus.submitted
this.application.preferences = []
this.application.language = this.contextApplication.language || Language.en

return this
}
Expand Down
6 changes: 2 additions & 4 deletions sites/public/pages/applications/start/autofill.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import {
FormCard,
ProgressNav,
UserContext,
blankApplication,
t,
} from "@bloom-housing/ui-components"
import { useForm } from "react-hook-form"
Expand Down Expand Up @@ -37,9 +36,8 @@ export default () => {
setSubmitted(true)
if (previousApplication && useDetails) {
conductor.application = previousApplication
} else {
conductor.application = blankApplication()
Copy link
Collaborator

@jaredcwhite jaredcwhite May 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I had this here so if somebody went back in their browser history and changed to not autofilling the application, it'd be rewritten as a blank application. Otherwise they could never undo their choice.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@jaredcwhite , how close do you think we are to being able to merge the Next branch? If you're thinking next week, then can we get this in first? We're in a tough spot of not wanting to delay the Next updates any longer, but we also don't want to stack up a lot of dependencies.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@seanmalbert Oh I want to get Next v10 ready to merge today, so I think this could follow suit.

}

context.syncApplication(conductor.application)
conductor.sync()
conductor.routeToNextOrReturnUrl()
Expand All @@ -61,7 +59,7 @@ export default () => {
)
if (data) {
if (data.items.length > 0) {
setPreviousApplication(new AutofillCleaner(data.items[0]).clean())
setPreviousApplication(new AutofillCleaner(data.items[0], application).clean())
} else {
onSubmit()
}
Expand Down
58 changes: 29 additions & 29 deletions sites/public/pages/applications/start/choose-language.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,11 @@ import {
ProgressNav,
UserContext,
t,
Form,
} from "@bloom-housing/ui-components"
import FormsLayout from "../../../layouts/forms"
import { useForm } from "react-hook-form"
import { AppSubmissionContext, retrieveApplicationConfig } from "../../../lib/AppSubmissionContext"
import React, { useContext, useEffect, useState } from "react"
import { Language } from "@bloom-housing/backend-core/types"

const loadListing = async (listingId, stateFunction, conductor, context) => {
const response = await axios.get(process.env.listingServiceUrl)
Expand All @@ -33,14 +32,15 @@ const loadListing = async (listingId, stateFunction, conductor, context) => {
export default () => {
const router = useRouter()
const [listing, setListing] = useState(null)
const [newLocale, setNewLocale] = useState("")
const context = useContext(AppSubmissionContext)
const { initialStateLoaded, profile } = useContext(UserContext)
const { conductor, application } = context

const listingId = router.query.listingId

useEffect(() => {
if (!listingId) return

if (!context.listing || context.listing.id != listingId) {
void loadListing(listingId, setListing, conductor, context)
} else {
Expand All @@ -52,10 +52,12 @@ export default () => {

const imageUrl = listing?.assets ? imageUrlFromListing(listing) : ""

/* Form Handler */
const { handleSubmit } = useForm()
const onSubmit = () => {
conductor.sync()
const onLanguageSelect = (language: Language) => {
conductor.currentStep.save({
language,
})

const newLocale = language == "en" ? "" : `/${language}`
void router.push(`${newLocale}${conductor.determineNextUrl()}`).then(() => {
window.scrollTo(0, 0)
})
Expand Down Expand Up @@ -93,28 +95,26 @@ export default () => {
)}

<div className="form-card__pager">
<Form className="" onSubmit={handleSubmit(onSubmit)}>
<div className="form-card__pager-row primary px-4">
{listing?.applicationConfig.languages.length > 1 && (
<>
<h3 className="mb-4 font-alt-sans field-label--caps block text-base text-black">
{t("application.chooseLanguage.chooseYourLanguage")}
</h3>

{listing.applicationConfig.languages.map((lang) => (
<Button
className="language-select mx-1"
onClick={() => {
setNewLocale(lang == "en" ? "" : `/${lang}`)
}}
>
{t(`applications.begin.${lang}`)}
</Button>
))}
</>
)}
</div>
</Form>
<div className="form-card__pager-row primary px-4">
{listing?.applicationConfig.languages.length > 1 && (
<>
<h3 className="mb-4 font-alt-sans field-label--caps block text-base text-black">
{t("application.chooseLanguage.chooseYourLanguage")}
</h3>

{listing.applicationConfig.languages.map((lang) => (
<Button
className="language-select mx-1"
onClick={() => {
onLanguageSelect(lang)
}}
>
{t(`applications.begin.${lang}`)}
</Button>
))}
</>
)}
</div>

{initialStateLoaded && !profile && (
<div className="form-card__pager-row primary px-4 border-t border-gray-450">
Expand Down