Fix/infinite loop #1234

dominikx96 · 2021-05-12T15:18:21Z

Issue

It's not covered by any issue.

Addresses # (issue)

This change addresses the issue in full
This change addresses only certain aspects of the issue
This change is a dependency for another issue
This change has a dependency from another issue

Description

This PR solves 2 problems:

A language from the choose-language step is not saved by a conductor. I also refactored autofill to do not reset context (language is selecting in the first step and autofill is a secondary step - so we need to know what language the user selected)

url: http://localhost:3000/applications/start/choose-language without listingId parameter occurs infinity loop with GET /listings call to the API.

Type of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update
This change requires a SQL Script

How Can This Be Tested/Reviewed?

Log in
Choose language different than EN (it was default previously so we want to test different case)
Submit application
Open public app in a new card (to reset context)
Log in
Post application with different language
See on the partners if an application includes selected language

Desktop View
Mobile View

Checklist:

netlify · 2021-05-12T15:23:49Z

Deploy Preview for clever-edison-cd22c1 ready!

Built with commit dcb8998

https://deploy-preview-1234--clever-edison-cd22c1.netlify.app

dominikx96 · 2021-05-12T15:45:26Z

@jaredcwhite I also noticed, ApplicationForms test doesn't have any sense after infinite loop fix. I need to figure out how to open this step with a correct listingId, then update the test. I think for now we can test all except this - I'll prepare fix soon

jaredcwhite

Hi @dominikx96, good catch on the application language not being saved. (I think there might have been a problem before where we never actually saved it.)

I have some concerns about this PR altering subtle behavior I fixed previously after discovering bugs. Can we basically just include the

     conductor.currentStep.save({
       language,
     })

change, and then for Autofill step just pass a language code directly as the second argument to the constructor?

Regarding the missing listing ID param issue, it looks like the fix in this PR is sort of broken:
https://deploy-preview-1234--clever-edison-cd22c1.netlify.app/applications/start/choose-language

I know in the past we had sort of hack in place which just pulled the first listing out of the result set. Maybe what we should just do instead is redirect to the home page if there's no listing ID param?

dominikx96 · 2021-05-17T11:39:33Z

Hi @dominikx96, good catch on the application language not being saved. (I think there might have been a problem before where we never actually saved it.)

I have some concerns about this PR altering subtle behavior I fixed previously after discovering bugs. Can we basically just include the
     conductor.currentStep.save({
       language,
     })
change, and then for Autofill step just pass a language code directly as the second argument to the constructor?

Regarding the missing listing ID param issue, it looks like the fix in this PR is sort of broken:
https://deploy-preview-1234--clever-edison-cd22c1.netlify.app/applications/start/choose-language

I know in the past we had sort of hack in place which just pulled the first listing out of the result set. Maybe what we should just do instead is redirect to the home page if there's no listing ID param?

@jaredcwhite agree I updated PR to just redirect the user to the /.

Unfortunately, I don't understand the first problem, could you explain it more with code samples?
Currently, I decided to pass a current application from the context as a second AutofillCleaner parameter in the constructor - it gives us more flexibility.

jaredcwhite

OK, I think there was just one specific place where a bug might have crept in again. Otherwise looks good to me—although will need a update for Next v10. Should we get that merged in and then this? Or merge this first?

jaredcwhite · 2021-05-17T21:26:35Z

sites/public/pages/applications/start/autofill.tsx

@@ -37,9 +36,8 @@ export default () => {
      setSubmitted(true)
      if (previousApplication && useDetails) {
        conductor.application = previousApplication
-      } else {
-        conductor.application = blankApplication()


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.

@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.

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

dominikx96 · 2021-05-20T16:24:49Z

@jaredcwhite I realized we can just create a new application object in the autofill step, even if autofill is available - the first step is choose-language, so it works as expected in both situations. Could you check it again? :)

jaredcwhite

@dominikx96 Approach looks good to me! 👍

jaredcwhite · 2021-05-21T00:52:51Z

sites/public/lib/appAutofill.ts

 } from "@bloom-housing/backend-core/types"
 import { blankApplication } from "@bloom-housing/ui-components"

 class AutofillCleaner {
  application: Application = null

+  // provide context value to override current application language choosen by user on the first step
+  // eslint-disable-next-line @typescript-eslint/no-explicit-any


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

nice catch! Removing it...

* Prevent infinity loop when listingId is not defined * Override language from the context * Updated test * Redirect to the homepage when there is no listingId * Updated cypress test * Updated autofill to do not override selected language * Cleanup Co-authored-by: dominikx96 <dominik@airnauts.com>

This reverts commit 5076466.

dominikx96 added 2 commits May 12, 2021 15:01

Prevent infinity loop when listingId is not defined

8987ddd

Override language from the context

d5b0b07

dominikx96 added the ready for review label May 12, 2021

dominikx96 requested a review from pbn4 May 12, 2021 15:18

dominikx96 self-assigned this May 12, 2021

Updated test

cf68ede

dominikx96 requested a review from jaredcwhite May 12, 2021 15:43

dominikx96 changed the title ~~Fix/infinity loop~~ Fix/infinite loop May 12, 2021

jaredcwhite requested changes May 14, 2021

View reviewed changes

Redirect to the homepage when there is no listingId

d1a729a

Updated cypress test

a33fade

pbn4 requested review from jaredcwhite and removed request for pbn4 May 17, 2021 15:02

seanmalbert assigned jaredcwhite May 17, 2021

jaredcwhite requested changes May 17, 2021

View reviewed changes

dominikx96 added 2 commits May 20, 2021 18:03

Updated autofill to do not override selected language

5697ea7

Merge branch 'master' into fix/infinity-loop

063bf15

dominikx96 requested a review from jaredcwhite May 20, 2021 16:24

jaredcwhite approved these changes May 21, 2021

View reviewed changes

Cleanup

dcb8998

dominikx96 merged commit eba88db into master May 24, 2021

dominikx96 mentioned this pull request May 24, 2021

Wait for checking listingId until router is ready #1261

Merged

27 tasks

willrlin referenced this pull request in CityOfDetroit/bloom Jun 9, 2021

Revert "Fix/infinite loop (#1234)"

71dd846

This reverts commit 5076466.

willrlin referenced this pull request in CityOfDetroit/bloom Jun 9, 2021

Revert "Fix/infinite loop (#1234)"

d9742fd

This reverts commit 5076466.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix/infinite loop #1234

Fix/infinite loop #1234

dominikx96 commented May 12, 2021 •

edited

Loading

netlify bot commented May 12, 2021 •

edited

Loading

dominikx96 commented May 12, 2021

jaredcwhite left a comment

dominikx96 commented May 17, 2021

jaredcwhite left a comment

jaredcwhite May 17, 2021 •

edited

Loading

seanmalbert May 18, 2021

jaredcwhite May 18, 2021

dominikx96 commented May 20, 2021

jaredcwhite left a comment

jaredcwhite May 21, 2021

dominikx96 May 21, 2021

Fix/infinite loop #1234

Fix/infinite loop #1234

Conversation

dominikx96 commented May 12, 2021 • edited Loading

Issue

Description

Type of change

How Can This Be Tested/Reviewed?

Checklist:

netlify bot commented May 12, 2021 • edited Loading

dominikx96 commented May 12, 2021

jaredcwhite left a comment

Choose a reason for hiding this comment

dominikx96 commented May 17, 2021

jaredcwhite left a comment

Choose a reason for hiding this comment

jaredcwhite May 17, 2021 • edited Loading

Choose a reason for hiding this comment

seanmalbert May 18, 2021

Choose a reason for hiding this comment

jaredcwhite May 18, 2021

Choose a reason for hiding this comment

dominikx96 commented May 20, 2021

jaredcwhite left a comment

Choose a reason for hiding this comment

jaredcwhite May 21, 2021

Choose a reason for hiding this comment

dominikx96 May 21, 2021

Choose a reason for hiding this comment

dominikx96 commented May 12, 2021 •

edited

Loading

netlify bot commented May 12, 2021 •

edited

Loading

jaredcwhite May 17, 2021 •

edited

Loading