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

Feat/buttons #585

Merged
merged 23 commits into from
Apr 10, 2023
Merged

Feat/buttons #585

merged 23 commits into from
Apr 10, 2023

Conversation

seaerchin
Copy link
Contributor

@seaerchin seaerchin commented Dec 7, 2022

tests coming in a follow up PR yeet

Problem

Previously, there was no api providing a translation from filename to staging url. This PR adds this service in.
Moreover, as part of providing this service, other tangential services have also been mildly reworked to accept a result-esque api.

Though this is rudimentary at present, we should refine this more in the future so that our APIs are safe.

also short writeup here for reasoning behind how service operates

Solution

  1. create a PageService that parses filenames into their respective types
    • done through recursive descent so that we minimise # of API calls needed as we fetch the full blob...
    • determination the type of a resource item still requires a full network call because the resource can be named differently from the page on github. This is most clear for file resources - the github file acts as a pointer to the actual content by means of file_url. For files/ext links, this should be fast b/c the actual file in the resource folder contains only metadata.
  2. Shift to ResultAsync based APIs rather than Promise based.
    • only done for Site/ReviewRequestService and only for some methods. The way this is done now is very crude and we should aim to return better errors rather than NotFoundError. This is just done as a base so that we can change the error type to be a more specific one in the future
    • the withError method in PageService should be extracted out in the future and used for interfacing with both .js files that call github APIs (either directly or indirectly through GithubService) and .ts files that call some external endpoints

Next steps

  • PageService should also aim to parse the page type (or defer it to some other FileService) as we have config files etc that are not tangentially related to what the user wants to see
  • Provide a method call to filter out files that are not directly related to the user (or otherwise provide an interface) + expensive filetypes (files/images).
    • expensive filetypes is to prevent meaninglesss blob comparisons

@seaerchin seaerchin marked this pull request as ready for review December 7, 2022 09:23
src/services/review/ReviewRequestService.ts Outdated Show resolved Hide resolved
src/errors/EmptyStringError.ts Outdated Show resolved Hide resolved
src/types/pages.ts Outdated Show resolved Hide resolved
src/types/pages.ts Show resolved Hide resolved
src/services/review/ReviewRequestService.ts Outdated Show resolved Hide resolved
src/routes/v2/authenticated/review.ts Outdated Show resolved Hide resolved
@alexanderleegs alexanderleegs force-pushed the feat/identity-phase-2 branch from 03610da to 1b0ae89 Compare March 8, 2023 06:30
@alexanderleegs alexanderleegs force-pushed the feat/identity-phase-2 branch from 1b0ae89 to 3db9533 Compare March 16, 2023 09:35
@alexanderleegs alexanderleegs force-pushed the feat/identity-phase-2 branch from eb89781 to d59f2d6 Compare March 30, 2023 03:45
Base automatically changed from feat/identity-phase-2 to develop March 30, 2023 04:36
* fix(sites.spec): update imports

* test(sitesservice): update types

* test(reviewrequestservice.spec): update spce

* test(collaboratorsservice): fixed tests

* chore(databaseerror): add new error

* fix(sitesservice): fixed regression in code

* test(sitesservice.spec): fixed tests

* chore(sitesservice): remove extra console log

* chore(sites/user spec): add sync to prevent db clash

* chore(databaseerror): removed extra properties

* chore(constants): add new constant for homepage name

* fix(pageservice): fixed bug allowing empty collections

* chore(constants): add new constant for contact-us

* refactor(pages): rename property

* test(pageservice.spec): add tests

* chore(constants): rename homepage constants for consistency

* chore(pageservice): remove redundant code

* chore(pageservice.spec,): remove extra async

* chore(pageservice.spec): standardise imports

\

* refactor(pageservice): update parsecolletionpage

* test(pageservice.spec): update tests for new error mesasge

* fix(pageservice.spec): removed extra newline

* test(integration): fixed reviews integration tests

* test(review): updated spec for new api

* tests(notifs + page): fixed failing tests

* test(review): fix for dev changes
@seaerchin seaerchin merged commit fe63434 into develop Apr 10, 2023
@seaerchin seaerchin deleted the feat/buttons branch April 10, 2023 10:08
@seaerchin seaerchin restored the feat/buttons branch April 11, 2023 06:05
@seaerchin seaerchin deleted the feat/buttons branch April 11, 2023 06:14
alexanderleegs pushed a commit that referenced this pull request Apr 11, 2023
* feat(reviewrequestservice): add initial impl to copute url

* chore(errors): add many errors

* feat(types): add types

* refactor(deployment): chaneg string type to permalink

* feat(pageservice): add new pageservice abstration for raw js wrappers

* feat(json): add new safeJsonParse util

* refactor(reviewrequestservice): refactor to use new page service

* refactor(sitesservice): refactor to use new page service

* refactor(review): refactor for neverthrow api

* chore(server): init page service

* chore(pageservice): add comment

* fix(reviewrequestservice): add in await

* fix(collaboratorsservice): add in unwrap

* chore(review.ts): removed vapt fixes

* chore(services/types): renamed variables and methods for clarity

* chore(errors): removed unneeded properties

* fix(server): u9pdate after rebase

* fix(review): update after rebase

* fix(rebase fixes): add cast cos the `permalink` property is guaranteed but it's opaque atm

* chore(server): add code for init of pageservice

* chore(rr service): specify return types

* fix(notif handler): update to fit new api

* test(app): updated specs to fit with new API   (#587)

* fix(sites.spec): update imports

* test(sitesservice): update types

* test(reviewrequestservice.spec): update spce

* test(collaboratorsservice): fixed tests

* chore(databaseerror): add new error

* fix(sitesservice): fixed regression in code

* test(sitesservice.spec): fixed tests

* chore(sitesservice): remove extra console log

* chore(sites/user spec): add sync to prevent db clash

* chore(databaseerror): removed extra properties

* chore(constants): add new constant for homepage name

* fix(pageservice): fixed bug allowing empty collections

* chore(constants): add new constant for contact-us

* refactor(pages): rename property

* test(pageservice.spec): add tests

* chore(constants): rename homepage constants for consistency

* chore(pageservice): remove redundant code

* chore(pageservice.spec,): remove extra async

* chore(pageservice.spec): standardise imports

\

* refactor(pageservice): update parsecolletionpage

* test(pageservice.spec): update tests for new error mesasge

* fix(pageservice.spec): removed extra newline

* test(integration): fixed reviews integration tests

* test(review): updated spec for new api

* tests(notifs + page): fixed failing tests

* test(review): fix for dev changes
@kishore03109 kishore03109 mentioned this pull request Apr 13, 2023
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.

2 participants