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

feat(rr): config parsing #662

Merged
merged 21 commits into from
May 25, 2023
Merged

feat(rr): config parsing #662

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
b6a0f8a
refactor(parsepagename): add guard for markdownpage
seaerchin Mar 17, 2023
60434f8
refactor(types/utils): update type def and shift util function otu
seaerchin Mar 17, 2023
b857a08
feat(config): add parsing
seaerchin Mar 20, 2023
ce13b7a
feat(pageservice): refactor to have stronger guarantee on method
seaerchin Mar 20, 2023
3f60d3b
chore(errors): add new error types
seaerchin Mar 22, 2023
75bd13d
chore(configService): add precondition for pathinfo
seaerchin Mar 22, 2023
7696bfa
refactor(types): rejig
seaerchin Mar 22, 2023
7213c8a
refactor(rr service): more elaborate parsing n defaults
seaerchin Mar 22, 2023
225b733
ref(review): update call sites and add new method
seaerchin Mar 22, 2023
38c5a5f
chore(services): init service
seaerchin Mar 22, 2023
aecfcff
fix(rr): change andThen to map
seaerchin Mar 22, 2023
11e8b84
chore(rr): ref to fiot new api
seaerchin Mar 22, 2023
f5d837c
chore(error): add status code
seaerchin Mar 22, 2023
7780a59
fix(sites): update typing and refactor for result
seaerchin Mar 22, 2023
31927e8
chore(review): update review dto
seaerchin Mar 27, 2023
d76f745
ref(types): add more github types
seaerchin Apr 6, 2023
0d4f6e5
chore(pages): add docs
seaerchin Apr 6, 2023
f6105e2
fix(tests): fixed tests and services
seaerchin May 17, 2023
42b9118
chore(notifications spec): add missing imports
seaerchin May 17, 2023
a12ca71
test(tests): fixed all tests
seaerchin May 17, 2023
ea8051b
chore(types): update types and add default return
seaerchin May 22, 2023
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
7 changes: 7 additions & 0 deletions src/errors/ConfigParseError.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import { BaseIsomerError } from "@root/errors/BaseError"

export default class ConfigParseError extends BaseIsomerError {
constructor(fileName: string) {
super(500, `The given file: ${fileName} was not a config file!`)
seaerchin marked this conversation as resolved.
Show resolved Hide resolved
}
}
9 changes: 9 additions & 0 deletions src/errors/NetworkError.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import { BaseIsomerError } from "@root/errors/BaseError"

export default class NetworkError extends BaseIsomerError {
constructor(
message = "An error occurred with the network whilst processing your request. Please try again later."
) {
super(500, message)
}
}
7 changes: 7 additions & 0 deletions src/errors/PageParseError.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import { BaseIsomerError } from "@root/errors/BaseError"

export default class PageParseError extends BaseIsomerError {
seaerchin marked this conversation as resolved.
Show resolved Hide resolved
constructor(fileName: string) {
super(500, `The given file: ${fileName} was not a page!`)
}
}
5 changes: 4 additions & 1 deletion src/integration/NotificationOnEditHandler.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import { ResourcePageService } from "@root/services/fileServices/MdPageServices/
import { SubcollectionPageService } from "@root/services/fileServices/MdPageServices/SubcollectionPageService"
import { UnlinkedPageService } from "@root/services/fileServices/MdPageServices/UnlinkedPageService"
import { CollectionYmlService } from "@root/services/fileServices/YmlFileServices/CollectionYmlService"
import { ConfigService } from "@root/services/fileServices/YmlFileServices/ConfigService"
import { FooterYmlService } from "@root/services/fileServices/YmlFileServices/FooterYmlService"
import { GitHubService } from "@services/db/GitHubService"
import * as ReviewApi from "@services/db/review"
Expand Down Expand Up @@ -100,14 +101,16 @@ const pageService = new PageService({
unlinkedPageService,
resourceRoomDirectoryService,
})
const configService = new ConfigService()
const reviewRequestService = new ReviewRequestService(
(mockGithubService as unknown) as typeof ReviewApi,
User,
ReviewRequest,
Reviewer,
ReviewMeta,
ReviewRequestView,
pageService
pageService,
configService
)
const sitesService = new SitesService({
siteRepository: Site,
Expand Down
6 changes: 4 additions & 2 deletions src/integration/Notifications.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ import {
} from "@root/fixtures/sessionData"
import { getAuthorizationMiddleware } from "@root/middleware"
import { NotificationsRouter as _NotificationsRouter } from "@root/routes/v2/authenticated/notifications"
import { SitesRouter as _SitesRouter } from "@root/routes/v2/authenticated/sites"
import { genericGitHubAxiosInstance } from "@root/services/api/AxiosInstance"
import { GitHubService } from "@root/services/db/GitHubService"
import { BaseDirectoryService } from "@root/services/directoryServices/BaseDirectoryService"
Expand All @@ -44,6 +43,7 @@ import { ResourcePageService } from "@root/services/fileServices/MdPageServices/
import { SubcollectionPageService } from "@root/services/fileServices/MdPageServices/SubcollectionPageService"
import { UnlinkedPageService } from "@root/services/fileServices/MdPageServices/UnlinkedPageService"
import { CollectionYmlService } from "@root/services/fileServices/YmlFileServices/CollectionYmlService"
import { ConfigService } from "@root/services/fileServices/YmlFileServices/ConfigService"
import { ConfigYmlService } from "@root/services/fileServices/YmlFileServices/ConfigYmlService"
import { FooterYmlService } from "@root/services/fileServices/YmlFileServices/FooterYmlService"
import CollaboratorsService from "@root/services/identity/CollaboratorsService"
Expand Down Expand Up @@ -101,14 +101,16 @@ const pageService = new PageService({
unlinkedPageService,
resourceRoomDirectoryService,
})
const configService = new ConfigService()
const reviewRequestService = new ReviewRequestService(
(gitHubService as unknown) as typeof ReviewApi,
User,
ReviewRequest,
Reviewer,
ReviewMeta,
ReviewRequestView,
pageService
pageService,
configService
)
const sitesService = new SitesService({
siteRepository: Site,
Expand Down
31 changes: 16 additions & 15 deletions src/integration/Reviews.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ import { ResourcePageService } from "@root/services/fileServices/MdPageServices/
import { SubcollectionPageService } from "@root/services/fileServices/MdPageServices/SubcollectionPageService"
import { UnlinkedPageService } from "@root/services/fileServices/MdPageServices/UnlinkedPageService"
import { CollectionYmlService } from "@root/services/fileServices/YmlFileServices/CollectionYmlService"
import { ConfigService } from "@root/services/fileServices/YmlFileServices/ConfigService"
import { FooterYmlService } from "@root/services/fileServices/YmlFileServices/FooterYmlService"
import { ReviewRequestDto } from "@root/types/dto/review"
import { GitHubService } from "@services/db/GitHubService"
Expand Down Expand Up @@ -132,14 +133,16 @@ const pageService = new PageService({
unlinkedPageService,
resourceRoomDirectoryService,
})
const configService = new ConfigService()
const reviewRequestService = new ReviewRequestService(
(gitHubService as unknown) as typeof ReviewApi,
User,
ReviewRequest,
Reviewer,
ReviewMeta,
ReviewRequestView,
pageService
pageService,
configService
)
const sitesService = new SitesService({
siteRepository: Site,
Expand Down Expand Up @@ -263,20 +266,20 @@ describe("Review Requests Integration Tests", () => {
const expected = {
items: [
{
type: ["page"],
type: "page",
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
cmsFileUrl: `${FRONTEND_URL}/sites/${MOCK_REPO_NAME_ONE}/homepage`,
lastEditedBy: MOCK_USER_EMAIL_TWO,
Copy link
Contributor

Choose a reason for hiding this comment

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

Sanity check here: is this removal of todo intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

lastEditedTime: new Date(MOCK_GITHUB_COMMIT_DATE_THREE).getTime(),
},
{
type: ["page"],
type: "page",
name: MOCK_GITHUB_FILENAME_ALPHA_TWO,
path: MOCK_GITHUB_FILEPATH_ALPHA_TWO.split("/").filter((x) => x),
stagingUrl: `${MOCK_DEPLOYMENT_DBENTRY_ONE.stagingUrl}${MOCK_PAGE_PERMALINK}`,
fileUrl: `${FRONTEND_URL}/sites/${MOCK_REPO_NAME_ONE}/editPage/${MOCK_GITHUB_FILENAME_ALPHA_TWO}`,
cmsFileUrl: `${FRONTEND_URL}/sites/${MOCK_REPO_NAME_ONE}/editPage/${MOCK_GITHUB_FILENAME_ALPHA_TWO}`,
lastEditedBy: MOCK_USER_EMAIL_TWO,
lastEditedTime: new Date(MOCK_GITHUB_COMMIT_DATE_THREE).getTime(),
},
Expand Down Expand Up @@ -750,22 +753,22 @@ describe("Review Requests Integration Tests", () => {
reviewRequestedTime: new Date(MOCK_GITHUB_DATE_ONE).getTime(),
changedItems: [
{
type: ["page"],
type: "page",
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
Copy link
Contributor

Choose a reason for hiding this comment

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

Sanity check again: checking in if this removal is intended

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

lastEditedBy: MOCK_USER_EMAIL_TWO,
lastEditedTime: new Date(MOCK_GITHUB_COMMIT_DATE_THREE).getTime(),
stagingUrl: `${MOCK_DEPLOYMENT_DBENTRY_ONE.stagingUrl}${MOCK_PAGE_PERMALINK}`,
cmsFileUrl: `${FRONTEND_URL}/sites/${MOCK_REPO_NAME_ONE}/homepage`,
},
{
type: ["page"],
type: "page",
name: MOCK_GITHUB_FILENAME_ALPHA_TWO,
path: MOCK_GITHUB_FILEPATH_ALPHA_TWO.split("/").filter((x) => x),
stagingUrl: `${MOCK_DEPLOYMENT_DBENTRY_ONE.stagingUrl}${MOCK_PAGE_PERMALINK}`,
fileUrl: `${FRONTEND_URL}/sites/${MOCK_REPO_NAME_ONE}/editPage/${MOCK_GITHUB_FILENAME_ALPHA_TWO}`,
lastEditedBy: MOCK_USER_EMAIL_TWO,
lastEditedTime: new Date(MOCK_GITHUB_COMMIT_DATE_THREE).getTime(),
stagingUrl: `${MOCK_DEPLOYMENT_DBENTRY_ONE.stagingUrl}${MOCK_PAGE_PERMALINK}`,
cmsFileUrl: `${FRONTEND_URL}/sites/${MOCK_REPO_NAME_ONE}/editPage/${MOCK_GITHUB_FILENAME_ALPHA_TWO}`,
},
],
}
Expand Down Expand Up @@ -1480,7 +1483,6 @@ describe("Review Requests Integration Tests", () => {
const actual = await request(app).post(
`/${MOCK_REPO_NAME_ONE}/${MOCK_GITHUB_PULL_REQUEST_NUMBER}/approve`
)
console.log(actual.error)

// Assert
expect(actual.statusCode).toEqual(403)
Expand All @@ -1498,7 +1500,6 @@ describe("Review Requests Integration Tests", () => {
const actual = await request(app).post(
`/${MOCK_REPO_NAME_ONE}/${MOCK_GITHUB_PULL_REQUEST_NUMBER}/approve`
)
console.log(actual.error)

// Assert
expect(actual.statusCode).toEqual(403)
Expand Down
5 changes: 4 additions & 1 deletion src/integration/Sites.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import { ResourcePageService } from "@root/services/fileServices/MdPageServices/
import { SubcollectionPageService } from "@root/services/fileServices/MdPageServices/SubcollectionPageService"
import { UnlinkedPageService } from "@root/services/fileServices/MdPageServices/UnlinkedPageService"
import { CollectionYmlService } from "@root/services/fileServices/YmlFileServices/CollectionYmlService"
import { ConfigService } from "@root/services/fileServices/YmlFileServices/ConfigService"
import { ConfigYmlService } from "@root/services/fileServices/YmlFileServices/ConfigYmlService"
import { FooterYmlService } from "@root/services/fileServices/YmlFileServices/FooterYmlService"
import IsomerAdminsService from "@root/services/identity/IsomerAdminsService"
Expand Down Expand Up @@ -89,14 +90,16 @@ const pageService = new PageService({
unlinkedPageService,
resourceRoomDirectoryService,
})
const configService = new ConfigService()
const reviewRequestService = new ReviewRequestService(
gitHubService,
User,
ReviewRequest,
Reviewer,
ReviewMeta,
ReviewRequestView,
pageService
pageService,
configService
)
const sitesService = new SitesService({
siteRepository: Site,
Expand Down
6 changes: 3 additions & 3 deletions src/routes/v2/authenticated/__tests__/Sites.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import express from "express"
import { ok, okAsync } from "neverthrow"
import { okAsync } from "neverthrow"
import request from "supertest"

import type { AuthorizationMiddleware } from "@middleware/authorization"
Expand Down Expand Up @@ -99,7 +99,7 @@ describe("Sites Router", () => {
describe("getStagingUrl", () => {
it("returns the site's staging URL", async () => {
const stagingUrl = "staging-url"
mockSitesService.getStagingUrl.mockResolvedValueOnce(ok(stagingUrl))
mockSitesService.getStagingUrl.mockReturnValueOnce(okAsync(stagingUrl))

const resp = await request(app)
.get(`/${mockSiteName}/stagingUrl`)
Expand Down Expand Up @@ -138,7 +138,7 @@ describe("Sites Router", () => {
stagingUrl: "staging-url",
siteUrl: "prod-url",
}
mockSitesService.getSiteInfo.mockResolvedValueOnce(ok(siteInfo))
mockSitesService.getSiteInfo.mockReturnValueOnce(okAsync(siteInfo))

const resp = await request(app).get(`/${mockSiteName}/info`).expect(200)

Expand Down
6 changes: 4 additions & 2 deletions src/routes/v2/authenticated/__tests__/review.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,10 +148,12 @@ describe("Review Requests Router", () => {
// Arrange
const mockFilesChanged = ["file1", "file2"]
mockIdentityUsersService.getSiteMember.mockResolvedValueOnce("user")
mockReviewRequestService.compareDiff.mockResolvedValueOnce(
mockFilesChanged
mockReviewRequestService.compareDiff.mockReturnValueOnce(
okAsync(mockFilesChanged)
)
mockSitesService.getBySiteName.mockResolvedValueOnce(ok(true))
mockSitesService.getStagingUrl.mockReturnValueOnce(okAsync("staging-url"))
mockGithubService.getRepoInfo.mockResolvedValueOnce(true)

// Act
const response = await request(app).get("/mockSite/review/compare")
Expand Down
19 changes: 12 additions & 7 deletions src/routes/v2/authenticated/review.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import autoBind from "auto-bind"
import express from "express"
import _ from "lodash"
import { ResultAsync } from "neverthrow"

import logger from "@logger/logger"

Expand All @@ -15,6 +14,8 @@ import UserWithSiteSessionData from "@classes/UserWithSiteSessionData"

import { CollaboratorRoles, ReviewRequestStatus } from "@root/constants"
import { SiteMember, User } from "@root/database/models"
import MissingSiteError from "@root/errors/MissingSiteError"
import { NotFoundError } from "@root/errors/NotFoundError"
import { GitHubService } from "@root/services/db/GitHubService"
import CollaboratorsService from "@root/services/identity/CollaboratorsService"
import NotificationsService from "@root/services/identity/NotificationsService"
Expand All @@ -25,10 +26,10 @@ import { ResponseErrorBody } from "@root/types/dto/error"
import {
CommentItem,
DashboardReviewRequestDto,
EditedItemDto,
UpdateReviewRequestDto,
ReviewRequestDto,
BlobDiffDto,
EditedItemDto,
} from "@root/types/dto/review"
import ReviewRequestService from "@services/review/ReviewRequestService"
// eslint-disable-next-line import/prefer-default-export
Expand Down Expand Up @@ -125,14 +126,18 @@ export class ReviewsRouter {
return this.sitesService
.getStagingUrl(userWithSiteSessionData)
.andThen((stagingLink) =>
seaerchin marked this conversation as resolved.
Show resolved Hide resolved
ResultAsync.fromSafePromise(
this.reviewRequestService.compareDiff(
userWithSiteSessionData,
stagingLink
)
this.reviewRequestService.compareDiff(
userWithSiteSessionData,
stagingLink
)
)
.map((items) => res.status(200).json({ items }))
.mapErr((err) => {
if (err instanceof MissingSiteError || err instanceof NotFoundError) {
return res.status(404).json({ message: err.message })
}
return res.status(500).json({ message: err.message })
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

})
}

createReviewRequest: RequestHandler<
Expand Down
Loading