-
Notifications
You must be signed in to change notification settings - Fork 87
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
Allow override of dataset badges #3189
base: master
Are you sure you want to change the base?
Allow override of dataset badges #3189
Conversation
This fix has been deployed in production to fix saving SPD datasets. |
Sure thing @maudetes! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much for this, indeed, it's a bit frightening to see we can break udata-front with what felt like basic changes on the backend 😱
Nice code, nice test, excellent work 💯
class DatasetBadge(Badge): | ||
kind = db.StringField(required=True, choices=list(BADGES.keys())) | ||
kind = db.StringField(required=True, validation=validate_badge) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using choices
makes it use a dropdown in swagger, which is convenient, but I don't know any existing way to have a dynamic list of choices... so I guess we have to let go of that convenience ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've actually tried keeping choices
with a lambda to compute the value dynamically but I could not make it work sadely
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, I tried also in the past, and couldn't make it work. There doesn't seem to be a way to do it, other than hacking/monkeypatching the lib itself, which seemed a little bit too much at the time.
I'll have a deeper look in the future if it's prioritized 👍
If we keep this direction, we may want to apply the same logic to other badges type |
Follows up on #3189, fixes [#1564](datagouv/data.gouv.fr#1564) --------- Co-authored-by: maudetes <[email protected]>
Fix datagouv/data.gouv.fr#1564
This is a quickfix for the dataset case, we may want to find a cleaner solution that apply to all.