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

Don't break profile page if a mod has no default_version #261

Merged

Conversation

DasSkelett
Copy link
Member

Problem

Currently, if for some, theoretically impossible, reason a mod of an author has no default_version, viewing their profile results in a 500.
(If the mod is still unpublished, it only results in a 500 for the author)

The cause of the error are the mod boxes, that are accessing mod.default_version.gameversion to display the For ... message in the corner. If mod.default_version is None, an exception is thrown.

Changes

mod-box.html checks if mod.default_version is None, and if so, displays an - instead of the KSP version.

Also the flask route is enhanced to also show unpublished mods to admins.

TODO

There are still many other places left where we are accessing mod.default_version without null-checks, for example the mod page itself.
They have to be fixed separately, which is more work.

If we want to allow mods without any releases, we'd have to do this work.
Otherwise, we could try to make the Mod.default_version_id column non-nullable. This needs a rather complex data migration to make sure there are no mods with default_version_id=None left, but I already have some code for that.

Most important, we have to find out, where Mod.default_version can be set to None currently and fix that.

@DasSkelett DasSkelett added Type: Bug Area: Backend Related to the Python code that runs inside gunicorn Priority: Normal Status: Ready Area: Frontend Related to HTML, JS, CSS, or other browser things labels May 15, 2020
Copy link

@Xinayder Xinayder left a comment

Choose a reason for hiding this comment

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

I think the most important part is trying to find out what can cause a mod without a default_version field. Otherwise, we'd have to extend this null check everywhere in the code.

I have an idea to test, maybe mods without default_version are created if the upload fails.

@HebaruSan
Copy link
Contributor

I think the most important part is trying to find out what can cause a mod without a default_version field.

In case anyone else has the same idea I had, you can't delete the default version:

if version[0].id == mod.default_version_id:
abort(400)

@HebaruSan HebaruSan requested a review from Xinayder May 16, 2020 00:02
Copy link

@Xinayder Xinayder left a comment

Choose a reason for hiding this comment

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

Just move the if block to hide the entire div if the mod doesn't have a default version 😄 .

@DasSkelett DasSkelett force-pushed the fix/show-unpublishe-to-admins branch from 74cfb1f to cbac8f4 Compare May 16, 2020 00:17
@DasSkelett
Copy link
Member Author

Should hide the full div now.

@DasSkelett DasSkelett merged commit d9370b8 into KSP-SpaceDock:alpha May 16, 2020
@DasSkelett DasSkelett deleted the fix/show-unpublishe-to-admins branch May 16, 2020 00:27
@HebaruSan HebaruSan mentioned this pull request Jun 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Backend Related to the Python code that runs inside gunicorn Area: Frontend Related to HTML, JS, CSS, or other browser things Priority: Normal Status: Ready Type: Bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants