CB-401, CB-408: Return 404 for entities that don't exist in the MB database #398
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Needed to prevent incompatibility with newer versions and our current version of flask
Removed in 13fc976 because we thought that the configuration was the same in both development and test due to the docker isolation, however the name of the musicbrainz database in test is different. We could have manually modified the config in each test base class, but it seems easier to just revert the test-specific config file.
CB-401 We originally had the ability to show a page about an "Unknown entity" if an MBID didn't exist in the database. This was useful when CB connected to an MB replica that may have been out of date by up to an hour. This would also allow us to show a review if an entity had since been deleted. Now CB connects directly to the main MB site, so this isn't an issue. For entities that have been deleted we decided to handle these on a case-by-case basis: CB-408 This commit also introduces the use of the brainzutils-mb-sample-database docker image for the musicbrainz data source. This is a real database with real data which allows us to test real/missing MBIDs without needing to mock methods or insert test data at the beginning of each test. It also makes test starup a bit faster.
This would fail if you requested a revision number greater than the number of revisions that a review had
This affects review pages, the browse review page, and the list of reviews by a user
This comment has been minimized.
This comment has been minimized.
Unit Test Results 1 files ±0 1 suites ±0 1m 16s ⏱️ ±0s For more details on these failures, see this check. Results for commit c7ad0e1. ± Comparison against base commit c7ad0e1. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a prerequisite to upgrading to BU 2.0 - we removed the concept of "unknown entities", and so had to make a decision about how to present these in CB.
Based on discussion in CB-401 and CB-408, we decided to explicitly return 404 on all entity pages that don't exist, and also on review pages for entities that don't exist.
In a future PR, we'll add an admin interface to see these items, and add the ability to change the entity id that the review is targeting, or delete the review.
In 754f5a7 I reverted a change that removed the test config file, because I realised that the test db name is different in tests and development.
Because create_app actually connects to the database in the config file, it's not possible to create the app and then change the config value and then connect to the database. Easier to revert the change.
I switched from using the metabrainz/musicbrainz-test-database:beta postgres image to metabrainz/brainzutils-mb-sample-database:schema-25-2021-04-04.0. This is much faster to start up (I was waiting ~10-15 seconds for the initial setup and schema creation every time I ran tests, now it's basically instant), and now the database has real musicbrainz data in it. This means that we were able to get rid of a bunch of mocks, which have always been a bit flaky for us. This involved changing a few entity mbids in tests to ones which were in the database.
Also fixed a bug in get_revision_number that showed up during testing, and pinned some dependency versions.