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 locations endpoint error when default translation is not public #2125

Merged
merged 1 commit into from
Mar 30, 2023

Conversation

seluianova
Copy link
Contributor

Short description

When a region creates a location which is a draft in the default language, but public in other languages, the location API returns an internal server error

Proposed changes

  • Disable "Publish" button in POI form for non-default languages, when default translation is not public
  • Exclude POIs without public translation in the default language from the locations API response
  • Add test data to cover the case

Side effects

Didn't find any

Resolved issues

Fixes: #2097


Pull Request Review Guidelines

@codeclimate
Copy link

codeclimate bot commented Mar 15, 2023

Code Climate has analyzed commit bca8ddb and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 83.3% (50% is the threshold).

This pull request will bring the total coverage in the repository to 76.1% (0.0% change).

View more on Code Climate.

Copy link
Contributor

@Gaston69 Gaston69 left a comment

Choose a reason for hiding this comment

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

Sounds good :-).
Peter and me suggest that you wanted to use continue instead of break to skip just the current POI instead of all the following.

integreat_cms/api/v3/locations.py Outdated Show resolved Hide resolved
integreat_cms/cms/templates/pois/poi_form.html Outdated Show resolved Hide resolved
Copy link
Member

@timobrembeck timobrembeck left a comment

Choose a reason for hiding this comment

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

Thanks a lot! 👍

This handles the existing incorrectly published translations in a way that prevents the API error, but I would like to add one of the following to make it easier for content creators to understand the change:

  1. Either create a migration that unpublishes all translations in this state, otherwise it might be confusing if they see "published" as status but they are not showing up in the app
  2. Or add a small warning in the page form view whenever a public location is not visible in the app because of a missing default translation

Apart from that, it raises a small UI/UX problem:

Screenshot 2023-03-16 at 22-59-31 Integreat Editorial System

The "draft" button looks exactly like the disabled "update" button. I think this is more the problem of the draft button than of the disabled button, so it's probably fine to merge this PR, but we should find a new color for non-disabled secondary buttons soon.

Edit: I opened a new ticket for this:

integreat_cms/api/v3/locations.py Outdated Show resolved Hide resolved
integreat_cms/cms/templates/pois/poi_form.html Outdated Show resolved Hide resolved
@seluianova seluianova force-pushed the bugfix/location-api-error branch 2 times, most recently from 85be968 to 41eacb4 Compare March 27, 2023 11:54
Copy link
Contributor Author

@seluianova seluianova left a comment

Choose a reason for hiding this comment

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

Added migration to update POI translation status to DRAFT, if there is no default public translation.
(hope I didn't miss anything, as I couldn't test it on real data).

It raises a small UI/UX problem

I noticed it too, but didn't come up with an idea for improvement.

Copy link
Member

@timobrembeck timobrembeck left a comment

Choose a reason for hiding this comment

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

Thanks a lot! 👍

Other than the performance optimization in the migration, I think this is good to go! 🚀

Co-authored-by: Timo Ludwig <timo.ludwig@tuerantuer.org>
@timobrembeck timobrembeck dismissed Gaston69’s stale review March 29, 2023 21:34

changes have been addressed

Copy link
Member

@ulliholtgrave ulliholtgrave left a comment

Choose a reason for hiding this comment

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

Thanks, Works as expected 👍

@seluianova seluianova merged commit 93405b2 into develop Mar 30, 2023
5 checks passed
@seluianova seluianova deleted the bugfix/location-api-error branch March 30, 2023 10:12
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.

Location endpoint raises internal server error when default translation is not public
4 participants