-
-
Notifications
You must be signed in to change notification settings - Fork 390
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
refactor: elevate download with donation to component level #3869
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,18 +1,96 @@ | ||
import '@testing-library/jest-dom/vitest' | ||
|
||
import { describe, expect, it } from 'vitest' | ||
import { fireEvent, render } from '@testing-library/react' | ||
import { describe, expect, it, vi } from 'vitest' | ||
|
||
import { render } from '../test/utils' | ||
import { Default } from './DownloadFileFromLink.stories' | ||
import { DownloadFileFromLink } from './DownloadFileFromLink' | ||
|
||
import type { DownloadFileFromLinkProps } from './DownloadFileFromLink' | ||
import type { IUploadedFileMeta } from './types' | ||
|
||
describe('DownloadFiles', () => { | ||
it('validates the component behaviour', () => { | ||
const { getByText } = render( | ||
<Default {...(Default.args as DownloadFileFromLinkProps)} />, | ||
const mockedUsedNavigate = vi.fn() | ||
vi.mock('react-router-dom', () => ({ | ||
useNavigate: () => mockedUsedNavigate, | ||
})) | ||
|
||
vi.mock('src/common/hooks/useCommonStores', () => ({ | ||
__esModule: true, | ||
useCommonStores: vi.fn(), | ||
})) | ||
|
||
describe('DownloadFileFromLink', () => { | ||
it('when logged out, requires users to login', () => { | ||
const { getAllByTestId } = render( | ||
<DownloadFileFromLink | ||
handleClick={vi.fn()} | ||
isLoggedIn={false} | ||
fileDownloadCount={0} | ||
fileLink="http://youtube.com/" | ||
files={[]} | ||
themeStoreDonationProps={{}} | ||
/>, | ||
) | ||
|
||
expect(getByText('Download files')).toBeInTheDocument() | ||
const downloadButton = getAllByTestId('downloadButton')[0] | ||
fireEvent.click(downloadButton) | ||
|
||
expect(mockedUsedNavigate).toHaveBeenCalledWith('/sign-in') | ||
}) | ||
|
||
it('when logged in, opens the donation modal for fileLink', () => { | ||
const handleClick = vi.fn() | ||
const fileLink = 'http://youtube.com/' | ||
|
||
const { getAllByTestId } = render( | ||
<DownloadFileFromLink | ||
handleClick={handleClick} | ||
isLoggedIn={true} | ||
fileDownloadCount={0} | ||
fileLink={fileLink} | ||
files={[]} | ||
themeStoreDonationProps={{}} | ||
/>, | ||
) | ||
|
||
const downloadButton = getAllByTestId('downloadButton')[0] | ||
fireEvent.click(downloadButton) | ||
|
||
expect(getAllByTestId('DonationRequestSkip')[0]).toHaveAttribute( | ||
'href', | ||
fileLink, | ||
) | ||
expect(getAllByTestId('DonationRequest')[0]).toBeInTheDocument() | ||
expect(handleClick).not.toHaveBeenCalled() | ||
}) | ||
|
||
it('when logged in, opens the donation modal for files', () => { | ||
const downloadUrl = 'http://great-url.com/' | ||
const handleClick = vi.fn() | ||
|
||
const { getAllByTestId } = render( | ||
<DownloadFileFromLink | ||
handleClick={handleClick} | ||
isLoggedIn={true} | ||
fileDownloadCount={0} | ||
fileLink={undefined} | ||
files={[ | ||
{ | ||
downloadUrl, | ||
name: 'first-file', | ||
size: 435435, | ||
} as IUploadedFileMeta, | ||
]} | ||
themeStoreDonationProps={{}} | ||
/>, | ||
) | ||
|
||
const downloadButton = getAllByTestId('downloadButton')[0] | ||
fireEvent.click(downloadButton) | ||
|
||
expect(getAllByTestId('DonationRequestSkip')[0]).toHaveAttribute( | ||
'href', | ||
downloadUrl, | ||
) | ||
expect(getAllByTestId('DonationRequest')[0]).toBeInTheDocument() | ||
expect(handleClick).not.toHaveBeenCalled() | ||
}) | ||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,28 +1,91 @@ | ||
import { useState } from 'react' | ||
import { useNavigate } from 'react-router-dom' | ||
|
||
import { DonationRequestModal } from '../DonationRequestModal/DonationRequestModal' | ||
import { DownloadButton } from '../DownloadButton/DownloadButton' | ||
import { ExternalLink } from '../ExternalLink/ExternalLink' | ||
import { DownloadCounter } from '../DownloadCounter/DownloadCounter' | ||
import { DownloadStaticFile } from '../DownloadStaticFile/DownloadStaticFile' | ||
|
||
import type { IThemeStoreDonationProps, IUploadedFileMeta } from './types' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IUploadedFileMeta can definitely be imported from shared. |
||
|
||
export interface DownloadFileFromLinkProps { | ||
link: string | ||
handleClick?: () => Promise<void> | ||
redirectToSignIn?: () => Promise<void> | ||
export interface IProps { | ||
handleClick: () => Promise<void> | ||
isLoggedIn: boolean | ||
fileDownloadCount: number | ||
fileLink: string | undefined | ||
files: (IUploadedFileMeta | File | null)[] | undefined | ||
Comment on lines
+15
to
+16
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As one or the other will be provided. Both should be optional. |
||
themeStoreDonationProps: IThemeStoreDonationProps | undefined | ||
} | ||
|
||
export const DownloadFileFromLink = (props: DownloadFileFromLinkProps) => { | ||
export const DownloadFileFromLink = (props: IProps) => { | ||
const { | ||
handleClick, | ||
fileDownloadCount, | ||
fileLink, | ||
files, | ||
isLoggedIn, | ||
themeStoreDonationProps, | ||
} = props | ||
const [isModalOpen, setIsModalOpen] = useState<boolean>(false) | ||
const [link, setLink] = useState<string>('') | ||
const navigate = useNavigate() | ||
|
||
const toggleIsModalOpen = () => setIsModalOpen(!isModalOpen) | ||
|
||
const callback = () => { | ||
handleClick() | ||
toggleIsModalOpen() | ||
} | ||
|
||
const filteredFiles: IUploadedFileMeta[] | undefined = files?.filter( | ||
(file): file is IUploadedFileMeta => file !== null && 'downloadUrl' in file, | ||
) | ||
|
||
return ( | ||
<> | ||
{!props.redirectToSignIn ? ( | ||
<ExternalLink | ||
href={props.link} | ||
onClick={() => props.handleClick && props.handleClick()} | ||
> | ||
<DownloadButton isLoggedIn onClick={() => {}} /> | ||
</ExternalLink> | ||
) : ( | ||
<DonationRequestModal | ||
body={themeStoreDonationProps?.body} | ||
callback={callback} | ||
iframeSrc={themeStoreDonationProps?.iframeSrc} | ||
imageURL={themeStoreDonationProps?.imageURL} | ||
isOpen={isModalOpen} | ||
link={link} | ||
onDidDismiss={() => toggleIsModalOpen()} | ||
/> | ||
|
||
{!isLoggedIn && ( | ||
<DownloadButton | ||
onClick={() => props.handleClick && props.handleClick()} | ||
onClick={() => navigate('/sign-in')} | ||
isLoggedIn={false} | ||
/> | ||
)} | ||
{isLoggedIn && ( | ||
<> | ||
{fileLink && ( | ||
<DownloadButton | ||
onClick={() => { | ||
setLink(fileLink) | ||
toggleIsModalOpen() | ||
}} | ||
isLoggedIn | ||
/> | ||
)} | ||
{filteredFiles && | ||
filteredFiles.map((file, index) => ( | ||
<DownloadStaticFile | ||
file={file} | ||
key={file ? file.name : `file-${index}`} | ||
handleClick={() => { | ||
setLink(file.downloadUrl) | ||
toggleIsModalOpen() | ||
}} | ||
forDonationRequest | ||
isLoggedIn | ||
/> | ||
))} | ||
</> | ||
)} | ||
<DownloadCounter total={fileDownloadCount} /> | ||
</> | ||
) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
export interface IUploadedFileMeta { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let me know your thoughts on this! The type is important, but since components are hosted in a separate package, I couldn't access the type in the src/pages package. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That type hasn't been updated in years, so it feels like there won't be too much maintenance burden between the two identical types There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Adding thoughts, I'd say two options:
Note: the *also I assume you were talking about the src/stores/storage.tsx - IUploadedFileMeta matching interface, if there are even more - more points towards "Clean clean clean". But would wait for some long time contributors to voice their thoughts too :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We're generally on a "clean, clean, clean" path atm. Speed lead approaches can be valid but that on this project has created too many issues as contributors change so the areas with technical debt/issues is knowledge not passed on. And yes, typings to 'oa-shared' is the current direction. #3789 does a lot of type updates, which we're about to merge in and will likely cause a bunch of conflicts with this branch, apologies. |
||
downloadUrl: string | ||
contentType?: string | null | ||
fullPath: string | ||
name: string | ||
type: string | ||
size: number | ||
timeCreated: string | ||
updated: string | ||
} | ||
|
||
export interface IThemeStoreDonationProps { | ||
body?: string | ||
iframeSrc?: string | ||
imageURL?: string | ||
} |
This file was deleted.
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'm not sure the right component has been updated here.
DownloadFileFromLink
certainly isn't right as it includes files now as well as links. I was quite happy with the naming ofDownloadWithDonationAsk
- does what it says on the tin.