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(openchallenges): remove usage of "Other" for platform #2393

Merged
merged 14 commits into from
Dec 14, 2023

Conversation

vpchung
Copy link
Member

@vpchung vpchung commented Dec 6, 2023

Fixes #2385

Changelog

  • remove "Other" from list of platforms
  • replace challenge.platform_id for "Other" (14) with NULL (\N)

Preview

Landscape now shows 13 platforms
Screenshot 2023-12-05 at 10 38 32 PM

"Other" no longer shows in platform facet
Screenshot 2023-12-05 at 10 38 48 PM

Platform info shows "Not available" in profiles for challenges hosted on non-OC platforms
Screenshot 2023-12-05 at 10 40 09 PM

@tschaffter
Copy link
Member

@vpchung What about representing missing values by an empty string in the OC Data sheet and settings the default value of the SQL property challenge.platform to NULL? If that works on the SQL side, this could help make the OC Data sheet less cluttered and reduce the size of the CSV files tracked on GitHub.

@vpchung
Copy link
Member Author

vpchung commented Dec 6, 2023

I don't think this will work, at least, not for the dump files to dump properly. If some rows contain less columns than other rows, then that row's data will be offset. But willing to try to see.

@vpchung vpchung requested a review from rrchai as a code owner December 6, 2023 19:45
@rrchai
Copy link
Contributor

rrchai commented Dec 6, 2023

The updates on API descriptions regarding of removing "Other" option of platform are pushed in this pr.

Initially, I considered removing the slug of the platform and allowing the challenge service to generate the slug automatically in the backend. However, this might also require removing the 'slug' column from the data.

To keep things simple, I've restricted the options to both the slug and name fields of the platform. @tschaffter Please let me know if there are any issues.

@tschaffter
Copy link
Member

Initially, I considered removing the slug of the platform...

What is the problem with the platform slug? Isn't it enough to set the challenge.platform property to nullable in the API spec?

@rrchai
Copy link
Contributor

rrchai commented Dec 6, 2023

What is the problem with the platform slug? Isn't it enough to set the challenge.platform property to nullable in the API spec?

It is enough with nullable challenge.platform. I thought we would like to limit options using these known platforms as a validation. If not restricting platform options using enum in API description, I think we should have a validation in the backend/frontend (in addition to data) to exclude unknown platforms?

@tschaffter
Copy link
Member

Because the platform data live in the DB, we shouldn't do any validation in the spec, which would require us to update the API description each time we add/update platform data in the DB.

@tschaffter
Copy link
Member

@vpchung Could you please resolve the conflict in challenges.csv? Thanks!

@vpchung
Copy link
Member Author

vpchung commented Dec 11, 2023

@tschaffter done ✅

@tschaffter tschaffter self-assigned this Dec 12, 2023
@tschaffter
Copy link
Member

tschaffter commented Dec 12, 2023

I updated the challenge service with the generator and weird things are happening like the challenge platform Java and TS object loosing some of its properties. Could this be a side effect of @rrchai using AllOf to make challenge.platform nullable? Currently investigating...

image

I also tried this solution described here (for OpenAPI 3.1) but this leads to the same behavior as when using Rong's solution (for OpenAPI 3.0 according to the SO post):

  platform:
    anyOf:
      - $ref: SimpleChallengePlatform.yaml
      - type: 'null'

Update: We are using OpenAPI 3.0.3! Previous attempts to move to OpenAPI 3.1 failed a few months ago at the level of code generation (see #1136). I should try again early next year.

@tschaffter
Copy link
Member

tschaffter commented Dec 14, 2023

@vpchung I fixed the styling issue visible on your screenshot where "Not Available" is not greyed out for platform. Now that platform is nullable AND optional, the property in TypeScript is:

platform?: SimpleChallengePlatform | null;

The styling check should check for these two values:

<td [ngClass]="{ 'text-grey': challenge.platform?.name === (undefined || null) }">

which I propose to simplify to the following:

<td [ngClass]="{ 'text-grey': !challenge.platform }">

Copy link
Member Author

@vpchung vpchung left a comment

Choose a reason for hiding this comment

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

Works on my end! Thanks, @rrchai @tschaffter

@tschaffter tschaffter merged commit 94c12dd into Sage-Bionetworks:main Dec 14, 2023
6 checks passed
@vpchung vpchung deleted the bug-2385 branch February 13, 2024 17:59
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.

[Bug] Set the platform to NULL for challenge that don't use a registered platform
3 participants