feat(rr): config parsing #662

seaerchin · 2023-03-22T09:31:23Z

Problem

Our current model of parsing is simplified and only works for pages; this PR adds parsing for configuration files (nav + setting)

Closes IS-54

Solution

add a config service that handles whether a file is a configuration file or not
extend dto definitions to differentiate between the types
in the event of failure, return the filename + folder path to the frontend to display

src/routes/v2/authenticated/sites.ts

src/errors/ConfigParseError.ts

src/errors/PageParseError.ts

harishv7 · 2023-03-27T06:00:29Z

src/services/fileServices/YmlFileServices/ConfigService.ts

+    return {
+      name,
+      path,
+      kind: "YmlConfig",


i like unions more tbh, i've ran into type errors w/ enums in the past due to how they are implemented in TS and hence, i've avoided them like the plague ever since.

see here or here for more details but if you feel strongly about this, i can change it

@seaerchin That's a good point! I think I used to do it like this:

enum ESomeEnum { ValOne: "1", ValTwo: "2" } type EFinalType = ESomeEnum.ValOne | ESomeEnum.ValTwo

Then we use the EFinalType for the actual assertion. Wdyt?

this actually results in the same thing HAHA

@seaerchin same thing as in the same issue described in the links above? how so?

oh ps, i haven't had time to look at this due to being caught up in the release but this has the same effect because the discriminated union still only allows the same set of values.

yes true, but felt it is cleaner with the use of discriminated unions + enums rather than strings

src/services/review/ReviewRequestService.ts

src/routes/v2/authenticated/review.ts

harishv7 · 2023-03-27T06:22:55Z

src/types/dto/review.ts

+}
+
+export interface EditedConfigDto extends BaseEditedItemDto {
+  type: "nav" | "setting"


using enums will be cleaner

see above ;--;

harishv7 · 2023-03-27T06:23:35Z

src/services/fileServices/YmlFileServices/ConfigService.ts

+import { Brand } from "@root/types/util"
+
+// eslint-disable-next-line import/prefer-default-export
+export class ConfigService {


do we have a test file for this new service?

not at present, will add in a future PR

src/services/review/ReviewRequestService.ts

src/utils/files.ts

kishore03109 · 2023-03-27T07:46:56Z

src/services/fileServices/MdPageServices/PageService.ts

+      .orElse(() => this.parseCollectionPage(pathInfo))
+
+  isMarkdownPage = (pageName: string): Result<string, PageParseError> => {
+    if (pageName.endsWith(".md")) return ok(pageName)


Nit: can we just return a boolean value instead? Using neverthrow here feels too verbose for a function like this

actually the problem here is that we want to distinguish MarkdownPages from other things (eg: YmlConfig or HtmlPages) - the problem with returning boolean is that we lose context wrt what the pageName is beyond its immediate containing function.

i can change this to string is MarkdownPage and brand this string to prevent the verbosity - wdyt

hmm isnt that beyond the scope of this function, which is to just tell if this page is of type MarkdownPages?
if not should we then rename this to getPageType right?

this is an example of a type guard and the naming is usually (by convention, at least) of the form isType

separately, telling if the page is of a specific type should assert that the page is that type, and not merely boolean otherwise downstream we would require calling again

kishore03109 · 2023-03-27T07:50:13Z

src/services/fileServices/MdPageServices/PageService.ts

+    ) {
+      return ok({ name: Brand.fromString(name), kind: "ContactUsPage" })
+    }
+    return err(new NotFoundError())


Do we want to return the specific file that wasnt found here? (ie the path?)

good point but actually i feel like we should extend NotFoundError to have a PageNotFoundError; the reason why idw to return just the pagename alone is cos we might have more errors in the future and no semantic meaning is tied to the path @__@

so like the page not found error would have a property page name, which can be used downstream to distinguish the type of page (if required) + provide a helpful message for displaying to FE.

didnt do here cos laze ._. (would have to do so for more errors to avoid inconsistency - maybe we can do this down the line)

src/services/fileServices/MdPageServices/PageService.ts

kishore03109 · 2023-03-27T08:00:28Z

CICD tests are failing, may I trouble you you to take a look at them?

seaerchin · 2023-03-27T08:34:50Z

CICD tests are failing, may I trouble you you to take a look at them?

kek i think the source branch is feat/buttons, which had tests fixed in #587 and #588; i'll probably rebase + merge those first and then rebase this on top once approved to see if the tests are still failing

kishore03109

[Tests] The full testing plan will be in the corresponding FE PR for buttons right? If not, please dismiss this review

kishore03109 · 2023-05-23T04:51:40Z

src/integration/Reviews.spec.ts

-            fileUrl: `${FRONTEND_URL}/sites/${MOCK_REPO_NAME_ONE}/homepage`,
-            lastEditedBy: MOCK_USER_EMAIL_TWO, // TODO: This should be MOCK_USER_EMAIL_ONE
+            cmsFileUrl: `${FRONTEND_URL}/sites/${MOCK_REPO_NAME_ONE}/homepage`,
+            lastEditedBy: MOCK_USER_EMAIL_TWO,


Sanity check here: is this removal of todo intended?

can i clarify this - isn't the removed property added back below?

kishore03109 · 2023-05-23T04:52:22Z

src/integration/Reviews.spec.ts

            name: MOCK_GITHUB_FILENAME_ALPHA_ONE,
            path: [],
-            stagingUrl: `${MOCK_DEPLOYMENT_DBENTRY_ONE.stagingUrl}${MOCK_PAGE_PERMALINK}`,
-            fileUrl: `${FRONTEND_URL}/sites/${MOCK_REPO_NAME_ONE}/homepage`,
-            lastEditedBy: MOCK_USER_EMAIL_TWO, // TODO: This should be MOCK_USER_EMAIL_ONE


Sanity check again: checking in if this removal is intended

can i clarify this - isn't the removed property added back below?

kishore03109 · 2023-05-23T04:53:51Z

src/routes/v2/authenticated/review.ts

+        if (err instanceof MissingSiteError || err instanceof NotFoundError) {
+          return res.status(404).json({ message: err.message })
+        }
+        return res.status(500).json({ message: err.message })


Should we return a generic error instead? Worried of the existance of errors bubbling up and sensitive info being leaked to our frontend

the NotFoundError was constructed either with no arguments (defaults to generic error messages of Something went wrong) or a default string (no sensitive info) or a formatted string containing the requested rawPath, similarly for MissingSiteError, it's just constructed with a default message of the requested site was not found in isomer, which i think is ok.

lmk if rawPath is a concern

src/types/pages.ts

seaerchin requested a review from a team March 22, 2023 09:34

seaerchin mentioned this pull request Mar 22, 2023

feat(rr): construct cms link for users #655

Merged

alexanderleegs reviewed Mar 22, 2023

View reviewed changes

src/routes/v2/authenticated/sites.ts Outdated Show resolved Hide resolved

seaerchin requested review from alexanderleegs and a team March 27, 2023 04:00

harishv7 reviewed Mar 27, 2023

View reviewed changes

kishore03109 reviewed Mar 27, 2023

View reviewed changes

seaerchin force-pushed the feat/edit-button branch from e03a740 to 4bc729a Compare April 12, 2023 08:40

Base automatically changed from feat/edit-button to develop April 17, 2023 04:29

seaerchin force-pushed the feat/config-parsing branch from 3ca54e8 to a496ee1 Compare May 17, 2023 10:05

seaerchin requested review from a team, kishore03109 and harishv7 May 17, 2023 10:07

alexanderleegs force-pushed the develop branch from ae3f22f to c1363db Compare May 19, 2023 01:31

seaerchin force-pushed the feat/config-parsing branch 3 times, most recently from 3cf6a41 to db69fa6 Compare May 23, 2023 03:53

kishore03109 approved these changes May 23, 2023

View reviewed changes

seaerchin added 10 commits May 24, 2023 18:16

refactor(parsepagename): add guard for markdownpage

b6a0f8a

refactor(types/utils): update type def and shift util function otu

60434f8

feat(config): add parsing

b857a08

feat(pageservice): refactor to have stronger guarantee on method

ce13b7a

chore(errors): add new error types

3f60d3b

chore(configService): add precondition for pathinfo

75bd13d

refactor(types): rejig

7696bfa

refactor(rr service): more elaborate parsing n defaults

7213c8a

ref(review): update call sites and add new method

225b733

chore(services): init service

38c5a5f

seaerchin added 11 commits May 24, 2023 18:17

fix(rr): change andThen to map

aecfcff

chore(rr): ref to fiot new api

11e8b84

chore(error): add status code

f5d837c

fix(sites): update typing and refactor for result

7780a59

chore(review): update review dto

31927e8

ref(types): add more github types

d76f745

chore(pages): add docs

0d4f6e5

fix(tests): fixed tests and services

f6105e2

chore(notifications spec): add missing imports

42b9118

test(tests): fixed all tests

a12ca71

chore(types): update types and add default return

ea8051b

seaerchin force-pushed the feat/config-parsing branch from db69fa6 to ea8051b Compare May 24, 2023 10:17

seaerchin mentioned this pull request May 25, 2023

feat(requestoverview): add link to staging/cms isomerpages/isomercms-frontend#1161

Merged

16 tasks

seaerchin merged commit b6fe4b7 into develop May 25, 2023

seaerchin deleted the feat/config-parsing branch May 25, 2023 05:09

This was referenced May 25, 2023

release(0.28.0): merge to develop #780

Merged

release(0.28.0): merge to prod #781

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(rr): config parsing #662

feat(rr): config parsing #662

seaerchin commented Mar 22, 2023 •

edited by jira bot

Loading

harishv7 Mar 27, 2023

seaerchin Mar 27, 2023

harishv7 Mar 29, 2023 •

edited

Loading

seaerchin Mar 29, 2023

harishv7 Mar 29, 2023

seaerchin Apr 6, 2023

harishv7 Apr 6, 2023

harishv7 Mar 27, 2023

seaerchin Mar 27, 2023

harishv7 Mar 27, 2023

seaerchin Mar 27, 2023

kishore03109 Mar 27, 2023

seaerchin Mar 27, 2023

kishore03109 Mar 28, 2023

seaerchin Mar 29, 2023

kishore03109 Mar 27, 2023

seaerchin Mar 27, 2023

kishore03109 commented Mar 27, 2023

seaerchin commented Mar 27, 2023

kishore03109 left a comment

kishore03109 May 23, 2023

seaerchin May 24, 2023

kishore03109 May 23, 2023

seaerchin May 24, 2023

kishore03109 May 23, 2023

seaerchin May 24, 2023

feat(rr): config parsing #662

feat(rr): config parsing #662

Conversation

seaerchin commented Mar 22, 2023 • edited by jira bot Loading

Problem

Solution

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harishv7 Mar 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kishore03109 commented Mar 27, 2023

seaerchin commented Mar 27, 2023

kishore03109 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seaerchin commented Mar 22, 2023 •

edited by jira bot

Loading

harishv7 Mar 29, 2023 •

edited

Loading