Dev.ej/partial xml in 422 assemble

[joanise]

PR Goal?

enable ReadAlongs/Studio-Web#463 in a better way: instead of hacking the full partial message into the exception message, return a JSONResponse with code 422, the usual detail field, and added fields for Studio-Web to handle if it wants.

Fixes?

one step along the path to better display g2p errors in S-Web

Also, I added spaces inside the quotes for the problem words, so we'll see ' ̓ ' instead of ' ̓' (desired change) and ' 234 ' instead of '234' (neutral change).

Note that I did not make the changes optional, since extra fields in the 422 response are currently ignored by Studio-Web.

Feedback sought?

• careful code review
• plus testing
• is the approach correct?

Priority?

normal

Tests added?

not yet, I have to think about how to do that.

The code is covered by existing tests but I will look for and I added explicit relevant assertions

How to test?

In Studio:

./run-web-api.sh

• Browse to http://127.0.0.1:8000/api/v1/docs#/default/assemble_assemble_post
• click Try it out
• paste diacritics ̓ are 123 foo : bar into the "input" part of the request
• click Execute
• You should get a 422 response with detail like in the production code, plus g2p_error_words with an array of unprocessable words and partial_ras with the RAS XML processed as far as it could be processed.

Contrast with doing the same on https://readalong-studio.herokuapp.com/api/v1/docs#/default/assemble_assemble_post where you will get only detail and no space inside the quoted g2p error words.

With the web API still running, go to Studio-Web and run:

npx nx serve studio-web

• Browse to http://localhost:4200/
• Enter diacritics ̓ are 123 foo : bar in the text box
• record random audio and click Go to the next step
• You should get a toast where the error words have a little bit of space so you can perceive the naked quote
• If you enable debugger tools and look at the Response for the 422 Assemble calls, you'll see g2p_error_words and partial_ras in there, which the Studio-Web app will ignore (until Del changes that).

Confidence?

Medium

Version change?

no

[semanticdiff-com]

Review changes with  

Changed Files

File	Status
readalongs/web_api.py	58% smaller
tests/test_web_api.py	31% smaller

[github-actions]

CLI load time: 0:00.05
Pull Request HEAD: 38b0ff463cf842f1d9580bf5c79f9d1af4274259
No imports take more than 0.1 s.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.59%. Comparing base (0a14465) to head (38b0ff4).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #292   +/-   ##
=======================================
  Coverage   88.59%   88.59%           
=======================================
  Files          22       22           
  Lines        1885     1885           
  Branches      309      309           
=======================================
  Hits         1670     1670           
  Misses        178      178           
  Partials       37       37           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[roedoejet]

This looks great @joanise

[sergeleger]

If you prefer the explicitness of HTTPException it should work also, since HTTPException.detail accepts any type:

raise HTTPException(status_code=422, detail={                     
    "detail": "g2p could not be performed, please check your text or your language code. " + logs,
     "g2p_error_words": non_convertible_words,
     "partial_ras": xml_to_string(g2ped),
})

But introduces a breaking change for Studio-Web, since the detail key with the error message is one level deeper in the response:

const resp = await fetch(...);
if(resp.status_code===422) {
   const exception = await resp.json();
   const contentLog = exception["detail"]["detail"];
   const g2pBadWords = exception["detail"]["g2p_error_words"];
}

[joanise]

Ah, I see. It's good to know for future similar cases, but backwards compatibility is important here to allow updating the back end and the front end each on their own schedule, so I'll keep the JSONResponse with a status code.

[sergeleger]

Looks good.