feat: interlinear export job ui#148
Conversation
arrocke
left a comment
arrocke
left a comment
There was a problem hiding this comment.
Thanks for your PR! It might be helpful to explain the big picture direction to guide your decision making. Long term this is likely going to end up as a weekly scheduled export job that exports these PDFs to a published location that we can link to on our site. I don't see us needing a wizard that let's you configure books and chapters. It will be enough to just include completed chapters
src/modules/export/react/InterlinearExportPanel.client.test.tsx
Outdated
Show resolved
Hide resolved
arrocke
left a comment
arrocke
left a comment
There was a problem hiding this comment.
Woops, meant to request changes. Let me know if you have any questions!
delgado-jacob
force-pushed
the
pdf_export/job_ui
branch
from
6b8ce9e to
f813466
Compare
arrocke
left a comment
arrocke
left a comment
There was a problem hiding this comment.
Thanks for the changes! A few cleanup things before we merge. In the future I would ask you to avoid rebasing and force merging once I've reviewed as it's much harder to see what you've changed.
| ): Promise<RequestInterlinearExportResult> { | ||
| const language = await resolveLanguageByCode(request.languageCode); | ||
| if (!language) { | ||
| throw new NotFoundError("Language not found."); |
There was a problem hiding this comment.
This error takes the resource type, not an error message
| throw new NotFoundError("Language not found."); | |
| throw new NotFoundError("Language"); |
There was a problem hiding this comment.
This use case can have a camelCase file name
|
|
||
| initializeDatabase(); | ||
|
|
||
| describe("requestInterlinearExport", () => { |
There was a problem hiding this comment.
We generally don't wrap tests in their own describe block since the file provides that sort of context already
| } from "../model"; | ||
| import jobRepository from "@/shared/jobs/JobRepository"; | ||
|
|
||
| export async function getInterlinearExportStatus(jobId: string) { |
There was a problem hiding this comment.
I'm not seeing this usecase used anywhere. Can we remove?
There was a problem hiding this comment.
Yes, left over from clean up. Removed.
@arrocke Sorry! I wasn't quite done following up. I intended to reply to each of your comments and ensure everything was addressed but I ran out of time and got pulled away this morning after pushing some changes. I'll follow up with the remainder of these most likely this weekend. |
|
Ah, my bad. I thought maybe you intended for me to review so I figured I'd at least see what changes you had addressed. When I saw that everything was complete, I left a review. |
Sounds good. Just habit since I work on several repos where the maintainers require all contributing PRs to always be rebased to head before merging. |
|
@arrocke I think I've addressed all your comments. Please let me know if anything else needs changed. Once this is merged, I'll open the upload placeholder PR. |
2 participants
Split from: #134
Introduces the feature‑flagged interlinear export UI plus job tracking, including new export module actions/use‑cases/query services/UI, job types and migration updates, admin nav wiring for the exports page, feature flag support, and associated tests