Skip to content

Comments

AP-7184b # draftService.syncDrafts to download draft data in the ba…#1023

Merged
mymattcarroll merged 6 commits intomasterfrom
AP-7184b
Feb 25, 2026
Merged

AP-7184b # draftService.syncDrafts to download draft data in the ba…#1023
mymattcarroll merged 6 commits intomasterfrom
AP-7184b

Conversation

@divporter
Copy link
Contributor

…ckground

Closes AP-7184

Requester Checklist

Please only check the items that you have actioned. Do not check items that are not applicable to your PR.

Implementation

  • Have you tested your implementation locally?
  • If applicable, has an appropriate changelog entry been added? Do not include if you are fixing/changing something that is only in the current release.
  • There are no warnings that have been suppressed unnecessarily
  • Have automated tests been added, or have related ones been updated to cover the change?
  • Have all OneBlink dependency updates been completed (eg apps / apps-react / types etc).
  • Have you ensured this change does not add unwanted dependencies?
  • If this PR contains a refactor, have relevant Jira testing tasks added?
  • Changes that will knowingly make the feature/bug incomplete have been commented with TODO and a description of what needs to be done to finish the feature/bug
  • Any changes made to public APIs have been reflected in the documentation
  • Have you isolated business logic where possible to allow for unit testing?

Logging and Debugging

  • Are the error messages, if any, informative?
  • Are there enough log events and are they written in a way that allows for easy debugging?
  • "Debugging" code removed
  • Front-end: No erroneous Console.WriteLines

Readability

  • All class, variable, property and method modifiers are provided with the smallest scope possible
  • New files, variables and functions are descriptive/comprehensible and named consistently.
  • There is no dead code (unreachable code)
  • There is no usage of magic numbers
  • There is no commented out code.
  • In hard-to-understand areas, comments exist and describe rationale or reasons for decisions in code

Security

  • All personal data inputs are checked (for the correct type, length/size, format, and range).
  • No sensitive information is logged or visible in a stacktrace
  • Are authorization and authentication handled correctly?
  • Is (user) input validated, sanitized, and escaped to prevent security attacks such as cross-site scripting or SQL injection?
  • Is data retrieved from external APIs or libraries checked for security issues?
  • Do API endpoints return appropriate status codes

Reviewer Guide

  • It is important that you understand the purpose of the PR.
  • You are encouraged to engage with the requestor if you do not understand any of the proposed code changes/additions/deletions.
  • Do you, the reviewer, understand what the code does? Do you think a specific expert, like a security expert or a usability expert, should look over the code before it can be accepted?
  • Is a framework, API, library, or service used that should not be used? Are there alternatives you could recommend?
  • Are there existing hooks/components/functions in the same code base that could be utilised?
  • When reviewing tests, attempt to identify missing edge cases that may be relevant to the proposed implementation
  • Ensure you check for:
    • Security
    • Scalability
    • Performance
    • Maintainability

@divporter divporter marked this pull request as draft February 25, 2026 01:20
@divporter divporter marked this pull request as ready for review February 25, 2026 03:13
Comment on lines 203 to +220
const draftSubmission = await getDraftSubmission(
formSubmissionDraft,
).catch((err) => {
console.warn(
`Could not fetch draft submission for draft: ${formSubmissionDraft.id}`,
err,
)

if (
err instanceof OneBlinkAppsError &&
err.title === DRAFT_DATA_UNAVAILABLE_ERROR_TITLE
) {
localFormSubmissionDraftsMap.set(formSubmissionDraft.id, {
...formSubmissionDraft,
downloadStatus: 'NOT_AVAILABLE',
})
return
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could change getDraftSubmission() to return undefined for missing data and change the other places its being called to throw the same error if it returns undefined, instead of relying on a string as the title. Please disregard if you like this implementation ❤️

Copy link
Contributor Author

@divporter divporter Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mymattcarroll mymattcarroll merged commit 2965493 into master Feb 25, 2026
1 check passed
@mymattcarroll mymattcarroll deleted the AP-7184b branch February 25, 2026 05:12
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