Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

SF-3089 Show message when draft could not be added to the chapter #3014

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

RaymondLuong3
Copy link
Collaborator

@RaymondLuong3 RaymondLuong3 commented Feb 13, 2025

If the user tries to add a draft to a chapter, it can fail if the internet connectivity is poor. This change notifies the user if a draft fails to be applied.

image


This change is Reviewable

@RaymondLuong3 RaymondLuong3 added the will require testing PR should not be merged until testers confirm testing is complete label Feb 13, 2025
await this.draftHandlingService.applyChapterDraftAsync(this.textDocId!, this.draftDelta);
this.isDraftApplied = true;
this.userAppliedDraft = true;
} catch {
Copy link
Collaborator

Choose a reason for hiding this comment

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

What if it isn't a network error? Can we make sure to handle those differently, so we still get reports in Bugsnag, and shown to the user?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Or just log everything to Bugsnag, even though network connection issues aren't really bugs. It would be nice to have something to refer to, just in case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, I have updated this to report errors to bugsnag silently unless it is a network related issue.

Copy link

codecov bot commented Feb 13, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.65%. Comparing base (7add4e4) to head (6f25dcf).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3014   +/-   ##
=======================================
  Coverage   82.65%   82.65%           
=======================================
  Files         553      553           
  Lines       32010    32016    +6     
  Branches     5186     5187    +1     
=======================================
+ Hits        26458    26464    +6     
  Misses       4795     4795           
  Partials      757      757           

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

Copy link
Collaborator

@josephmyers josephmyers left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Nateowami)

await this.draftHandlingService.applyChapterDraftAsync(this.textDocId!, this.draftDelta);
this.isDraftApplied = true;
this.userAppliedDraft = true;
} catch {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Or just log everything to Bugsnag, even though network connection issues aren't really bugs. It would be nice to have something to refer to, just in case.

@josephmyers josephmyers self-assigned this Feb 17, 2025
@RaymondLuong3 RaymondLuong3 force-pushed the fix/sf-3089-re-add-draft branch from d0cd9c6 to 6f25dcf Compare February 19, 2025 18:09
Copy link
Collaborator Author

@RaymondLuong3 RaymondLuong3 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @josephmyers and @Nateowami)

await this.draftHandlingService.applyChapterDraftAsync(this.textDocId!, this.draftDelta);
this.isDraftApplied = true;
this.userAppliedDraft = true;
} catch {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, I have updated this to report errors to bugsnag silently unless it is a network related issue.

@josephmyers
Copy link
Collaborator

I am good with this is if @Nateowami is

@josephmyers josephmyers assigned Nateowami and unassigned josephmyers Feb 25, 2025
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
will require testing PR should not be merged until testers confirm testing is complete
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants