Closed easherma closed 4 years ago
@desigonz to write up issue for the regression + test case with acceptance criteria
Braindump for places to test:
Unit tests: For model/page:
For the API (kinda systems test?):
changed the estimate as the original estimate was just to document the regression test, but we really need to be able to fix the problem to unblock #3682 and future features
Ideal test case: Every rich text field has an internal or doc link ServicePage with steps steps include a steps_with_location block There is also at least once component included from 'dynamicContent' There are also several steps with options, which have nested options (which are rich text and should have a test internal link field or document link field)
It passes if: All links are expanded html All data shows up in graphql in the structure expected Janis still has everything it needs to render related information, etc
We've got three main options I can see:
1) https://github.com/cityofaustin/joplin/tree/3567-api-rep-error <--- strips out the custom service page stuff, fixes some errors, has related location info in pretty much the same structure
2) https://github.com/cityofaustin/joplin/pull/541 Leaves more code in place, sorta works. Dosen't work (i.e. links are not expanded in rich text blocks) for nested streamfields (such as steps with options), This was the first option I tried to go with to fix the bug, but it wasn't as much a path of least resistance as I'd hoped.
As is, it's going to require a good chunk more boilerplate code to be added to make all the Streamfield options work for service pages ( we'll basically need to write resolvers for all kinds of Streamfields + make sure they work when arbitrarily nested and still return expanded links, etc).
3) Third option would be to really give wagtail-grapple a solid try, as most of the above boilerplate would be already in place, we'd have a more solid foundation to build off of for the API vs refactoring our stuff completely from 'scratch'. I think this is the best long term option. Ideally we'd be able to introduce grapple in place alongside our current API. I haven't gotten this to work yet, but haven't sunk a ton of time into it yet.
I want to do more testing on the first option, especially with Janis, and see what @briaguya thinks. But at the moment if I had a pick a best course of action it would be #1 to restore links working for service page streamfields, followed by #3 once we have some breathing room
Ready-ish to test @: https://janis-3567-richtext-links.netlify.com
https://joplin-pr-3567-api-rep-error.herokuapp.com/
Service page with a bunch of kinds of steps and links everywhere: https://janis-3567-richtext-links.netlify.com/en/health-safety/health-prevention/disease-prevention/get-immunized/
https://joplin-pr-3567-api-rep-error.herokuapp.com/admin/pages/9/edit/#tab-content
The testing I've done is still manual, haven't added unit or systems tests (still thinking about what the best way to do that might be)
I've mostly focused on the functionality of this bug, but since we changed some of the ways Janis is querying data, it's possible we might also want to check other contextual nav stuff? ( I'd defer to @briaguya on other potential places to check for other regressions)
Test criteria: Any kind of link(internal, external, document link, etc) added to a rich text widget should work as the correct janis link, even if its in a streamfield on a service page. ( more on that here: https://github.com/cityofaustin/techstack/issues/3852#issuecomment-582638760 )
I added links to and published the follow pages on Joplin:
Can you look into how the Janis and Joplin branches are hooked up?
I was able to trigger the builds via Netlify and test out the cases above.
QA passed for the test cases! Looks and works great. We can spin out writing unit tests into a different issue, but use those test cases as the basis for those unit tests.
@easherma Content has been sharing a lot of internal links that aren't working on published pages, such as this one on the mobile food vendor guide. Do you have an estimate on when this will be merged? Weighing whether to edit the pages with external links or waiting on this fix to be in production.
Here's a nice regression that i discovered while working on #3682 https://github.com/cityofaustin/techstack/issues/3053 ( https://github.com/cityofaustin/joplin/pull/420 ) Did its thing, QA'ed, merged https://github.com/cityofaustin/techstack/issues/3504 (https://github.com/cityofaustin/joplin/pull/499) Did its thing, code change merged in actually undoes the fix of the previous commit
So you might notice that internal links inside rich text fields in streamfields don't work anymore, but I don't think we noticed at the time that the new code merged in was undoing a previous fix and the new code wasn't incorporating the old code :tada:
Hoping to sort this out as part of the changes for #3682, but we should probably also have a regression test to cover cases like this (in progress here: https://github.com/cityofaustin/joplin/pull/545)
Two main goals: