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

Jump to bottom

hotfix(0.18.1): merge into develop #674

Merged
merged 3 commits into from
Mar 31, 2023
Merged

hotfix(0.18.1): merge into develop #674

merged 3 commits into from
Mar 31, 2023

Conversation

seaerchin
Copy link
Contributor

@seaerchin seaerchin commented Mar 31, 2023 •
edited
Loading

Problem

  1. old sites that are on github but not on db are mistakenly returned as 404 as the db query is empty
  2. DOMPurify too restrictive on BE

Solution

  1. check data on github in addition to checking sites.
  2. use same setup as FE

@seaerchin seaerchin requested a review from a team March 31, 2023 04:07
@seaerchin seaerchin mentioned this pull request Mar 31, 2023
Merged
kishore03109
kishore03109 previously approved these changes Mar 31, 2023
View reviewed changes
Copy link
Contributor

@kishore03109 kishore03109 left a comment

Choose a reason for hiding this comment

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

Nits:
we shouldnt have an additional call to github if the site already exists in our db.
But since this is non-functional and more of an optimisation, we can tackle this after our hotfix!

src/routes/v2/authenticated/review.ts Outdated Show resolved Hide resolved
@seaerchin
Copy link
Contributor Author

seaerchin commented Mar 31, 2023 •
edited
Loading

Nits: we shouldnt have an additional call to github if the site already exists in our db. But since this is non-functional and more of an optimisation, we can tackle this after our hotfix!

yea but then it becomes q hard to read cos you have nested ifs

harishv7
harishv7 reviewed Mar 31, 2023
View reviewed changes
src/routes/v2/authenticated/review.ts
} catch {
// If there are any errors, we failed to fetch the site
// and the site is assumed to not exist.
return false
Copy link
Contributor

Choose a reason for hiding this comment

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

in the event site does not exist, what response does the github API return?

Also, when we say errors, do we mean any response apart from 2xx goes into the catch block?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

returns 404 on our side - this means that it falls through to the default case where the site really doesn't exist

re: second point, yes. broadly, if it's a 4xx, we can omit the response from github because it's user based so our subsequent flow should cover it. if it's truly github's fault (5xx), we can't determine if the site exists or not, so returning that it doesn't is a safer choice than saying it does (and possibly being wrong)

Copy link
Contributor

Choose a reason for hiding this comment

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

okay makes sense

src/routes/v2/authenticated/review.ts Outdated Show resolved Hide resolved
@seaerchin seaerchin requested a review from a team March 31, 2023 07:51
@seaerchin seaerchin dismissed kishore03109’s stale review March 31, 2023 07:58

stale, got another bugfix

@seaerchin seaerchin force-pushed the hotfix/0.18.0 branch 2 times, most recently from bdede53 to 9c3df90 Compare March 31, 2023 08:13
@seaerchin
fix(review): return 200 for unmigrated sites
bd69c29 
chore(review): remove check on POST route

tests(review): fixed tests

tests(integration): fixed review tests

chore(review): update function name

fix(reviews): update spy

fix(review): check for migrated site but not site members
@seaerchin
fix(sanitize): use same setup for dompurify as FE
c25f448 
fix(markdown): correct .split
@harishv7 harishv7 merged commit 9499912 into develop Mar 31, 2023
2 checks passed
@mergify mergify bot deleted the hotfix/0.18.0 branch March 31, 2023 10:04
@alexanderleegs alexanderleegs restored the hotfix/0.18.0 branch April 3, 2023 02:22
@seaerchin seaerchin mentioned this pull request Apr 6, 2023
Merged
@seaerchin seaerchin deleted the hotfix/0.18.0 branch March 28, 2024 05:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kishore03109 kishore03109 kishore03109 approved these changes

@harishv7 harishv7 Awaiting requested review from harishv7

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@seaerchin @harishv7 @kishore03109