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

3007: Fix feeback rating for no search results as negative #3036

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+90 −11
Open

3007: Fix feeback rating for no search results as negative #3036

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
15 changes: 13 additions & 2 deletions native/src/components/FeedbackContainer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,17 @@ export type FeedbackContainerProps = {
cityCode: string
query?: string
slug?: string
noResults: boolean
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
noResults: boolean
noResults?: boolean

I think there is noResults only when I search and 0 results so it's false most of the time until we set it to true.

}

const FeedbackContainer = ({ query, language, routeType, cityCode, slug }: FeedbackContainerProps): ReactElement => {
const FeedbackContainer = ({
query,
language,
routeType,
cityCode,
slug,
noResults,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
noResults,
noResults = false,

}: FeedbackContainerProps): ReactElement => {
const [comment, setComment] = useState<string>('')
const [contactMail, setContactMail] = useState<string>('')
const [isPositiveRating, setIsPositiveRating] = useState<boolean | null>(null)
Expand All @@ -33,7 +41,10 @@ const FeedbackContainer = ({ query, language, routeType, cityCode, slug }: Feedb

useEffect(() => {
setSearchTerm(query)
}, [query])
if (noResults === true) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (noResults === true) {
if (noResults) {

it's already a boolean

setIsPositiveRating(false)
}
Comment on lines +44 to +46
Copy link
Member

Choose a reason for hiding this comment

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

🔧 If possible, it is always nice to avoid code in useEffects to prevent unexpected side effects and make the code and control structure easier to read and follow. In this case, it is not necessary to update setIsPositiveRating on every render since it is only possible to send feedback if there are no results at all (and noResults is therefore true). Therefore, initializing the corresponding useState with noResults ? false : null for isPositiveRating should be enough I think 🤔

In any case, checking explicitly for === true should not be necessary.

}, [query, noResults])

const handleSubmit = () => {
setSendingStatus('sending')
Expand Down
74 changes: 68 additions & 6 deletions native/src/components/__tests__/FeedbackContainer.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,12 @@ describe('FeedbackContainer', () => {

const city = 'augsburg'
const language = 'de'
const noResults = false
Copy link
Contributor

Choose a reason for hiding this comment

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

As long you add the default value false for noResults this is not needed

Suggested change
const noResults = false


it('should send feedback request with rating and no other inputs on submit', async () => {
const { getByText, findByText } = render(
<NavigationContainer>
<FeedbackContainer routeType={CATEGORIES_ROUTE} language={language} cityCode={city} />
<FeedbackContainer routeType={CATEGORIES_ROUTE} language={language} cityCode={city} noResults={noResults} />
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<FeedbackContainer routeType={CATEGORIES_ROUTE} language={language} cityCode={city} noResults={noResults} />
<FeedbackContainer routeType={CATEGORIES_ROUTE} language={language} cityCode={city} />

</NavigationContainer>,
)
const positiveRatingButton = getByText('useful')
Expand Down Expand Up @@ -70,7 +71,7 @@ describe('FeedbackContainer', () => {
const contactMail = '[email protected]'
const { getByText, findByText, getAllByDisplayValue } = render(
<NavigationContainer>
<FeedbackContainer routeType={CATEGORIES_ROUTE} language={language} cityCode={city} />
<FeedbackContainer routeType={CATEGORIES_ROUTE} language={language} cityCode={city} noResults={noResults} />
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<FeedbackContainer routeType={CATEGORIES_ROUTE} language={language} cityCode={city} noResults={noResults} />
<FeedbackContainer routeType={CATEGORIES_ROUTE} language={language} cityCode={city} />

</NavigationContainer>,
)
const [commentField, emailField] = getAllByDisplayValue('')
Expand Down Expand Up @@ -106,7 +107,7 @@ describe('FeedbackContainer', () => {
it('should disable send feedback button if rating button is clicked twice', async () => {
const { getByText, findByText } = render(
<NavigationContainer>
<FeedbackContainer routeType={CATEGORIES_ROUTE} language={language} cityCode={city} />
<FeedbackContainer routeType={CATEGORIES_ROUTE} language={language} cityCode={city} noResults={noResults} />
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<FeedbackContainer routeType={CATEGORIES_ROUTE} language={language} cityCode={city} noResults={noResults} />
<FeedbackContainer routeType={CATEGORIES_ROUTE} language={language} cityCode={city} />

</NavigationContainer>,
)
const positiveRatingButton = getByText('useful')
Expand All @@ -120,7 +121,13 @@ describe('FeedbackContainer', () => {
const query = 'Zeugnis'
const { findByText, getByText } = render(
<NavigationContainer>
<FeedbackContainer routeType={SEARCH_ROUTE} language={language} cityCode={city} query={query} />
<FeedbackContainer
routeType={SEARCH_ROUTE}
language={language}
cityCode={city}
query={query}
noResults={noResults}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
noResults={noResults}

/>
</NavigationContainer>,
)
const button = getByText('send')
Expand All @@ -145,7 +152,13 @@ describe('FeedbackContainer', () => {
const fullSearchTerm = 'Zeugnisübergabe'
const { findByText, getByDisplayValue, getByText } = render(
<NavigationContainer>
<FeedbackContainer routeType={SEARCH_ROUTE} language={language} cityCode={city} query={query} />
<FeedbackContainer
routeType={SEARCH_ROUTE}
language={language}
cityCode={city}
query={query}
noResults={noResults}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
noResults={noResults}

/>
</NavigationContainer>,
)
const input = getByDisplayValue(query)
Expand All @@ -170,12 +183,61 @@ describe('FeedbackContainer', () => {
it('should disable send button if query term is removed', async () => {
const { findByText, getByDisplayValue } = render(
<NavigationContainer>
<FeedbackContainer routeType={SEARCH_ROUTE} language={language} cityCode={city} query='query' />
<FeedbackContainer
routeType={SEARCH_ROUTE}
language={language}
cityCode={city}
query='query'
noResults={noResults}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
noResults={noResults}

/>
</NavigationContainer>,
)
expect(await findByText('send')).not.toBeDisabled()
const input = getByDisplayValue('query')
fireEvent.changeText(input, '')
expect(await findByText('send')).toBeDisabled()
})

it('should send negative rating on submit if there are no search results found', async () => {
const query = 'gesundheitsversicherung'
const noResults = true
const { getByText, findByText } = render(
<NavigationContainer>
<FeedbackContainer
routeType={SEARCH_ROUTE}
language={language}
cityCode={city}
query={query}
noResults={noResults}
/>
</NavigationContainer>,
)
expect(getByText('send')).not.toBeDisabled()
const submitButton = getByText('send')
fireEvent.press(submitButton)
expect(await findByText('thanksMessage')).toBeDefined()
expect(mockRequest).toHaveBeenCalledTimes(1)
expect(mockRequest).toHaveBeenCalledWith({
routeType: SEARCH_ROUTE,
isPositiveRating: false,
city,
language,
comment: '',
contactMail: '',
query,
searchTerm: query,
slug: undefined,
})
expect(sendTrackingSignal).toHaveBeenCalledTimes(1)
expect(sendTrackingSignal).toHaveBeenCalledWith({
signal: {
name: SEND_FEEDBACK_SIGNAL_NAME,
feedback: {
positive: false,
numCharacters: 0,
contactMail: false,
},
},
})
})
})
2 changes: 1 addition & 1 deletion native/src/routes/FeedbackModalContainer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ type FeedbackModalContainerProps = {
}

const FeedbackModalContainer = ({ route }: FeedbackModalContainerProps): ReactElement => (
<FeedbackContainer {...route.params} />
<FeedbackContainer {...route.params} noResults={false} />
Copy link
Contributor

Choose a reason for hiding this comment

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

noResults is optional you can just add noResults = false at the FeedbackContainer props and remove this..

Suggested change
<FeedbackContainer {...route.params} noResults={false} />
<FeedbackContainer {...route.params} />

)

export default FeedbackModalContainer
8 changes: 7 additions & 1 deletion native/src/routes/SearchModal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,13 @@ const SearchModal = ({
accessibilityLabel={t('searchResultsCount', { count: searchResults.length })}
style={{ flex: 1 }}
noItemsMessage={
<FeedbackContainer routeType={SEARCH_ROUTE} language={languageCode} cityCode={cityCode} query={query} />
<FeedbackContainer
routeType={SEARCH_ROUTE}
language={languageCode}
cityCode={cityCode}
noResults={searchResults.length === 0}
query={query}
/>
}
/>
</>
Expand Down
2 changes: 1 addition & 1 deletion web/src/components/FeedbackContainer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ export const FeedbackContainer = ({
query,
slug,
searchTerm,
isPositiveRating,
isPositiveRating: noResults === true ? false : isPositiveRating,
Copy link
Member

@steffenkleinle steffenkleinle Jan 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

🙃 Could you try to simplify this (using the && operator)? Furthermore in any case, checking explicitly for === true should not be necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
isPositiveRating: noResults === true ? false : isPositiveRating,
isPositiveRating: noResults ? false : isPositiveRating,

})

setSendingStatus('successful')
Expand Down
Loading