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 PostPreviewSnapshot rendering #8475

Merged
merged 1 commit into from
Jun 14, 2024
Merged

Fix PostPreviewSnapshot rendering #8475

merged 1 commit into from
Jun 14, 2024

Conversation

chosak
Copy link
Member

@chosak chosak commented Jun 14, 2024

Commit 942be70 broke rendering of PostPreviewSnapshots because the post preview template now expects a serialized version of pages, not the page objects themselves.

I've updated the relevant code and added a new regression test that actually renders the PPS template, something we lacked before.

How to test this PR

To test, run a local server and visit

http://localhost:8000/compliance/amicus/
http://localhost:8000/rules-policy/notice-opportunities-comment/

These pages now once again render properly.

Notes and todos

It's strange and suboptimal that SublandingPages with PostPreviewSnapshots don't use the same page search approach that regular FilterableLists do. For example, this page looks like a FilterableList but under the hood it's using this get_browsefilterable_posts method that does a direct database query (with some smelly logic) instead of querying the page search index. Is there any reason why we shouldn't try to unify this code? Ping @csebianlander for any thoughts.

Checklist

  • PR has an informative and human-readable title
  • Changes are limited to a single goal (no scope creep)
  • Code follows the standards laid out in the CFPB development guidelines

Commit 942be70 broke rendering of
PostPreviewSnapshots because the post preview template now expects a
serialized version of pages, not the page objects themselves.

I've updated the relevant code and added a new regression test that
actually renders the PPS template, something we lacked before.

To test, run a local server and visit

http://localhost:8000/compliance/amicus/
http://localhost:8000/rules-policy/notice-opportunities-comment/

These pages now once again render properly.
@chosak chosak requested a review from willbarton June 14, 2024 16:55
@chosak
Copy link
Member Author

chosak commented Jun 14, 2024

Is there any reason why we shouldn't try to unify this code?

Looking at this again I see this line which filters out pages with "archive" in the title. There must be some specific need for this, but still it seems like we could use the regular filterable list logic and tack on some extra filtering afterwards instead of implementing a totally separate lookup.

@chosak chosak added this pull request to the merge queue Jun 14, 2024
@willbarton
Copy link
Member

Is there any reason why we shouldn't try to unify this code?

While I think unifying this code would make sense, I think it's worth stepping back and asking: should sublanding pages be behaving this much like sublanding filterable pages? It's effectively a limited filterable list page without the filterable controls, and I question whether that's a useful thing to exist alongside our actual filterable list page types.

Maybe this is worth adding to our filterable list backlog, @sonnakim @csebianlander .

Merged via the queue into main with commit 2f2e726 Jun 14, 2024
12 checks passed
@chosak chosak deleted the fix/post-preview-snapshot branch June 14, 2024 17:29
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.

2 participants