Skip to content

Feature/tech 6338/download dq kit#240

Open
gauravgupta-nagarro wants to merge 3 commits into
mainfrom
feature/TECH-6338/download-dq-kit
Open

Feature/tech 6338/download dq kit#240
gauravgupta-nagarro wants to merge 3 commits into
mainfrom
feature/TECH-6338/download-dq-kit

Conversation

@gauravgupta-nagarro
Copy link
Copy Markdown
Collaborator

What type of PR is this?

  • build: Commits that affect build components like build tool, dependencies, project
    version
  • chore: Miscellaneous commits (e.g. modifying .gitignore)
  • ci: Commits are special build commits that affect the CI/CD pipeline
  • docs: Commits that affect documentation only
  • feat: Commits that add a new feature
  • fix: Commits that fix a bug
  • perf: Commits are special refactor commits that improve performance
  • refactor: Commits that rewrite/restructure your code, however does not change any
    behaviour
  • revert: Commits that revert another commit/PR, usually can be autogenerated on
    GitHub or using git revert
  • style: Commits are special refactor commits that edit the code to comply with a
    code style, linter, or formatter
  • test: Commits that add missing tests or correcting existing tests

Summary

What does this PR do

How to test

  1. Instructions on how to test
  2. Specify which files to review
  3. etc.

Link to Jira/Asana/Airtable task (if applicable)

placeholder

Wireframe screenshot/screencap (if applicable)

placeholder

Implementation screenshot/screencap (if applicable)

placeholder

buffer.seek(0)
return buffer

logger.info("Pre-built DQ Kit not found. Building on-demand.")
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

If the Dagster prebuilt dq kit is not found, it's just returning None and on uploads.py will break. I think there is not on-demand build or any fallback case

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@Javiershenbc for old pipelines on demand zip was not required as per the requirements, so let me know how we want to handle this, just hide the button?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I would just hide the button in that case, so there is less confusion for the user. In case of future changes, we can manage this small change.

cc: @brianmusisi

</Tabs>
</div>

{uploadData.dq_status === "COMPLETED" && (
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This should be only for school geolocation uploads. There should be no map for coverage or other uploads. Either we add a dataset = geolocation o more ideally if the backend can flag it as having or not a map so the frontend doesnt have to guess

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

done


{mapUrl && !mapLoading && !mapError && (
<iframe
src={mapUrl}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The map is rendered as active HTML outside the frontend bundle, so I’d treat iframe sandboxing as a small hardening improvement. Adding sandbox="allow-scripts" would allow the map JavaScript to run while keeping it isolated from the parent app context.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

done

Copy link
Copy Markdown
Collaborator

@Javiershenbc Javiershenbc left a comment

Choose a reason for hiding this comment

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

Added some comments for improvements.

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