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

refactor: convert CsvMergedHeadersGenerator to typescript #2080

Merged
merged 35 commits into from
Jun 22, 2021

Conversation

chowyiyin
Copy link
Contributor

Problem

This PR converts the CsvMergedHeadersGenerator to typescript.

Closes #2030

Solution

Apart from converting the CsvMergedHeadersGenerator class to typescript, the response classes in csv-response-classes were also converted to typescript. Also, the Response class was converted to an abstract class, which cannot be instantiated. All subclass will have to implement the functions getAnswer and numCols. This was done as the existing implementation in CsvMergedHeadersGenerator implies the need for all subclasses of Response to have the two aforementioned functions.

Manual Tests

  • Create a storage mode form with a table field (to check TableResponse), a checkbox field (to check ArrayAnswerResponse) and an email field (to check SingleAnswerResponse). Submit a few responses and download the csv. Ensure that all answers to each form field are present in the csv and the metadata headers are also present.

@chowyiyin chowyiyin marked this pull request as draft June 7, 2021 03:39
@chowyiyin chowyiyin marked this pull request as ready for review June 7, 2021 04:42
Copy link
Contributor

@mantariksh mantariksh left a comment

Choose a reason for hiding this comment

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

I think we should type getResponseInstance as part of this as well, wdyt?

src/types/response/index.ts Outdated Show resolved Hide resolved
src/public/modules/forms/helpers/response-factory.ts Outdated Show resolved Hide resolved
src/public/modules/forms/helpers/response-factory.ts Outdated Show resolved Hide resolved
src/public/modules/forms/helpers/response-factory.ts Outdated Show resolved Hide resolved
src/public/modules/forms/helpers/response-factory.ts Outdated Show resolved Hide resolved
src/public/modules/forms/helpers/response-factory.ts Outdated Show resolved Hide resolved
src/public/modules/forms/helpers/response-factory.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@mantariksh mantariksh left a comment

Choose a reason for hiding this comment

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

pinging @karrui for another pair of eyes

@karrui
Copy link
Contributor

karrui commented Jun 16, 2021

Can we also add more test cases with the additional response type checks?

@chowyiyin chowyiyin requested a review from karrui June 21, 2021 02:50
Copy link
Contributor

@karrui karrui left a comment

Choose a reason for hiding this comment

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

lgtm but wait for @mantariksh's approval too

@chowyiyin chowyiyin requested a review from mantariksh June 21, 2021 04:01
@chowyiyin chowyiyin merged commit 4a91084 into develop Jun 22, 2021
@chowyiyin chowyiyin deleted the refactor/CsvMergedHeadersGenerator branch June 22, 2021 09:08
@karrui karrui mentioned this pull request Jun 24, 2021
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.

Convert CsvMergedHeadersGenerator class and associated functions to TypeScript
4 participants