GDCC/8924 - Check archival status#8925
Conversation
which gets displayed
|
[My first foray into QA] It looks good overall. the plain vanilla path does what it purports to do. If there are normal exceptions - dataset not found, version not available, user not found, user without proper permissions there are good error messages returned. The only thing that I can see that needs to be addressed is that a JsonParsingException is not being caught and so the response code is 500 and the user only gets empty braces in the console. Looking at the logs reveals the exception. |
|
Is that new? I would guess it is from and the framework returning 500 before our code is invoked. If so, I could change to having the method start with a string so we can parse the initial json ourselves and return a better error.So - basically the question is - is the use case sending bad json in or sending in valid json that, after html tags are stripped, becomes bad? |
|
You're right. It's not new. The other methods in Datasets that catch the parsing exceptions are taking in strings and parsing them inside the method. Let me think about having a case where stripping the tags would yield a parsing exception - if that could happen. |
|
My guess is that's hard to do, but I can add a change to use string and catch the parse exception post sanitization. (It probably makes more sense to avoid converting to Json twice anyway.) |
What this PR does / why we need it: Remove any html tags from supplied archival status (new APIs being added in 5.12). Also updates the Jsoup class we use in general.
Which issue(s) this PR closes:
Closes #8924
Special notes for your reviewer:
FWIW: The Jsoup update appears to just be a classname change but the docs do say the old class will be retired soon.
Suggestions on how to test this: Use the archival status API (must be superuser) to put an HTML message and confirm the result from doing a get (or what's displayed) does not include html.
Does this PR introduce a user interface change? If mockups are available, please link/include them here:
Is there a release notes update needed for this change?:
Additional documentation: