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

fix(#1205): deprem io response fix #1213

Conversation

absolutezero13
Copy link
Contributor

@absolutezero13 absolutezero13 commented Feb 28, 2023

Description

  • I've changed replaceAll function to replace because node v14 doesn't support replaceAll. Parser was crashing on the server side because of this.
  • I've added fizikiDurum property for the description as it looks like the most suitable to me. But we can change it, since the bigger problem is solved, that's why I wanted to create this PR.

discord username: afetharita#0001

closes #issue #1205 #1210

Please describe your changes. Also describe your aim and content. Do not forget to list the dependencies required caused by those changes.

## Things to check before creating a PR

  • I have inspected my topic, checked.
  • If it is a core feature, executed detailed tests.

Creating PR rules

  • PR must be created for an issue with approved tag. Otherwise PR will be rejected.
  • Relevant issue number: The issue number related to PR must be attached to head of PR header, after prefix after prefix # must be attached in parenthesis. A header like this could be used "prefix(#issue_number): PR header" .
  • A descriptive and understandable title: The PR title should clearly describe the nature and purpose of the changes. The PR title should be the first thing displayed when the PR is opened. And it should follow the semantic commit rules. For example, a title like "docs(#issueId): Add README.md" can be used.
  • Related file selection: Only relevant files should be touched and no other files should be affected.
  • Format and Lint Suitability : The code should be made in accordance with a certain format standard and examined according to the lint rules.
  • Clean commit history: The commits where the changes are made should be clear and organized.
  • Opening PR when job is completed: PR should be opened when job is completed and sent for review by other team members.

Changes

  • A new feature (a change that adds a new feature, not a breaking change)
  • A new refactor (a change that is not a breaking change, that improves the readability or performance of the code)
  • A breaking change
  • Documentation change

How were these changes tested?

Please describe the tests you did to test the changes you made. Please also specify your test configuration.

Test Configuration:

  • Firmware version:
  • Hardware:
  • Toolchain:
  • SDK:

@absolutezero13 absolutezero13 requested a review from a team February 28, 2023 20:05
@vercel
Copy link

vercel bot commented Feb 28, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
deprem-yardim-frontend ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Mar 3, 2023 at 9:05AM (UTC)

@yusufmeteyilmaz
Copy link

LGTM!

@yusufmeteyilmaz
Copy link

I'm sorry, I was rash.
There is a problem.
Not every request returns the same response, I mean some of them contain insufficient or irrelevant information.
I think we should use "full_text".

For example:
id:1614534
Api: https://apigo.afetharita.com/feeds/1614534
Afet harita: https://afetharita.com/en?id=1614534
Screenshot:
image
Rc: https://rc.afetharita.com/en?id=1614534
Screenshot:
image
vercel preview: https://deprem-yardim-frontend-6xmb3tl96-afet.vercel.app/en?id=1614534
Screenshot:
image
Response:
image
image

@absolutezero13
Copy link
Contributor Author

@hackerone-yusuf I've changed it to full_text. Can you check?

@yusufmeteyilmaz
Copy link

yusufmeteyilmaz commented Feb 28, 2023

Yes, it works as expected now, thanks.
But I saw the email info on one of them, I guess this kind of personal information should be filtered again.
Should I open a new issue for that?

@absolutezero13
Copy link
Contributor Author

Yes, it works as expected now, thanks. But I saw the email info on one of them, I guess this kind of personal information should be filtered again. Should I open a new issue for that?

Hm, I am not sure about this to be honest. But I don't think there should be another issue since this is not merged.

@yusufmeteyilmaz
Copy link

@absolutezero13 Alright then, thank you again.

@usirin
Copy link
Contributor

usirin commented Mar 3, 2023

... because node v14 doesn't support replaceAll.

i am confused, why are we making a change for node 14?

@absolutezero13
Copy link
Contributor Author

@usirin I'm not sure where but while setting up this project there was a spesific node version requirement that is around v14 or something. But if it's node v15 or above in production then it will be fine but even then there is no need to use replaceAll when there is regex.

@@ -16,7 +16,7 @@ import { useReasoningFilterMenuOption, useTimeStamp } from "@/stores/urlStore";

const parseExtraParams = (extraParamsStr: string) => {
return dJSON.parse<string, ChannelData["properties"]>(
extraParamsStr?.replaceAll("nan", "")
extraParamsStr?.replace("nan", "")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess this is gonna only will replace the first "nan" that it find. It does not seem like a fix.

More than that, why are we replacing "nan" to "" already on frontend side? Let's fix it on backend

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure

Copy link
Collaborator

@eraygundogmus eraygundogmus Mar 3, 2023

Choose a reason for hiding this comment

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

@absolutezero13 It would be great if you could open an issue on backend, thank you. I am closing this PR, you can reopen it if you think that is wrong and anyone can reconsider! thanks for contributing

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.

4 participants