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: migrate admin-console client service to ts #1531

Merged
merged 17 commits into from
Apr 7, 2021

Conversation

seaerchin
Copy link
Contributor

@seaerchin seaerchin commented Mar 31, 2021

Problem

Closes #1521 - refactors admin-console.client.service.js into two separate frontend services and writes adds tests for them

Solution

Rewrote the admin-console service as two separate services in typescript.
Followed backend structure and structured it as exampleService and billingService

Tests

1.tests for billingService/exampleService to ensure that api requests are being carried out and to ensure that errors are being thrown if promises are rejected

Manual tests

  • Click on examples after logging in - there should be multiple examples loaded
  • Login and create a form. Navigate to share and then click copy on Share Template. Open the copied url in another tab - the form should open as a template.
  • Login and navigate to billing.
    1. entering an invalid eserviceId should have 0 logins
    2. entering a valid eserviceId should have the logins for the specified time period.

@liangyuanruo liangyuanruo changed the title Ref/migrate admin console client service to ts refactor: migrate admin-console client service to ts Mar 31, 2021
Copy link
Contributor

@liangyuanruo liangyuanruo left a comment

Choose a reason for hiding this comment

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

the JSDoc for getExampleForms is incomplete because i'm unsure how to describe them. the corresponding backend controller examples.controller doesn't have the documentation defined on the parameter/type. will ask around and write it up

Thanks, noted. Appreciate it if you could derive and fill in the backend JSDocs, if they're missing too?

Meanwhile, you should actually switch over to using the TS implementation and delete the old JS files. Don't forget to test on staging if you have to.

@seaerchin
Copy link
Contributor Author

@liangyuanruo yep, i'm going to check with either @karrui or @mantariksh and fill in both fe/be documentation

src/public/services/BillingService.ts Outdated Show resolved Hide resolved
src/public/services/ExampleService.ts Outdated Show resolved Hide resolved
@seaerchin seaerchin requested review from karrui and liangyuanruo April 1, 2021 03:48
@seaerchin seaerchin force-pushed the ref/migrate-admin-console-client-service-to-ts branch from 1b87df9 to 85a7641 Compare April 1, 2021 03:53
@seaerchin seaerchin force-pushed the ref/migrate-admin-console-client-service-to-ts branch from 85a7641 to 300f708 Compare April 1, 2021 04:14
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.

some queries

src/public/services/__tests__/BillingService.test.ts Outdated Show resolved Hide resolved
src/public/services/__tests__/BillingService.test.ts Outdated Show resolved Hide resolved
src/types/api/billing.ts Outdated Show resolved Hide resolved
src/app/modules/billing/billing.controller.ts Show resolved Hide resolved
@seaerchin seaerchin requested a review from karrui April 6, 2021 10:09
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, please add manual test steps for next release

@karrui
Copy link
Contributor

karrui commented Apr 7, 2021

This PR may have conflicts with #1573, please coordinate :P
@orbitalsqwib FYI

@seaerchin
Copy link
Contributor Author

This PR may have conflicts with #1573, please coordinate :P
@orbitalsqwib FYI

talked to @orbitalsqwib on this. we finalized that i'll merge my PR in first and he'll rebase/merge off this accordingly. @karrui fyi

Copy link
Contributor

@liangyuanruo liangyuanruo left a comment

Choose a reason for hiding this comment

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

Code LGTM, do address/resolve Antariksh's comments before merging.

@seaerchin seaerchin merged commit 324a510 into develop Apr 7, 2021
@karrui karrui mentioned this pull request Apr 13, 2021
@karrui karrui deleted the ref/migrate-admin-console-client-service-to-ts branch April 21, 2021 02:16
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 admin-console.client.service.js to TypeScript
4 participants