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: schemas for upload API #29604

Merged
merged 1 commit into from
Jul 16, 2024
Merged

fix: schemas for upload API #29604

merged 1 commit into from
Jul 16, 2024

Conversation

betodealmeida
Copy link
Member

SUMMARY

Make sure get_schema_access_for_file_upload returns a set, so it can be used by the API.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

Added unit test.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@dosubot dosubot bot added the need:tests This PR requires tests label Jul 16, 2024
Copy link
Contributor

@Vitor-Avila Vitor-Avila left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@sadpandajoe sadpandajoe left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -993,7 +993,7 @@ def get_schema_access_for_file_upload( # pylint: disable=invalid-name
self, g.user
)
allowed_databases += extra_allowed_databases
return sorted(set(allowed_databases))
Copy link
Member

Choose a reason for hiding this comment

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

@betodealmeida Do you know why it was sorted before?

Copy link
Member Author

Choose a reason for hiding this comment

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

It was sorted so it can be displayed nicely in the UI, but since that's a UI-only concern we've been moving the sorting logic to the frontend. I'm going to update it next, for now this PR fixes only this because it's broken by returning a list.

Copy link
Member

@michael-s-molina michael-s-molina Jul 16, 2024

Choose a reason for hiding this comment

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

Should the frontend change also happen in this PR to make it atomic? This would make the PR not only fix the list issue but also preserve the current sorted behavior. In other words, there would not be any side effect as the result of this PR except the fix.

@michael-s-molina michael-s-molina added the review:checkpoint Last PR reviewed during the daily review standup label Jul 16, 2024
@betodealmeida betodealmeida merged commit b66c0f8 into master Jul 16, 2024
33 checks passed
@rusackas rusackas deleted the fix-schemas-for-upload branch July 16, 2024 21:08
@rusackas rusackas removed the review:checkpoint Last PR reviewed during the daily review standup label Jul 17, 2024
eschutho pushed a commit that referenced this pull request Jul 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need:tests This PR requires tests size/S
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants