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

Skip confirmation step on email subscription #1646

Merged
merged 12 commits into from
Jun 29, 2023

Conversation

Jaskon
Copy link
Contributor

@Jaskon Jaskon commented Jun 25, 2023

When user is logged in with the same email he tries to subscribe
According to #1174

When user logged in with the same email he tries to subscribe
…-sub-confirm

# Conflicts:
#	frontend/apps/remark42/app/common/api.ts
set autoConfirm param to make it work
@Jaskon Jaskon requested a review from umputun as a code owner June 25, 2023 00:10
@codecov
Copy link

codecov bot commented Jun 25, 2023

Codecov Report

Patch coverage: 90.00% and project coverage change: +0.08 🎉

Comparison is base (07667c8) 58.63% compared to head (6b001f2) 58.71%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1646      +/-   ##
==========================================
+ Coverage   58.63%   58.71%   +0.08%     
==========================================
  Files         128      128              
  Lines        2877     2885       +8     
  Branches      730      697      -33     
==========================================
+ Hits         1687     1694       +7     
- Misses       1065     1187     +122     
+ Partials      125        4     -121     
Impacted Files Coverage Δ
frontend/apps/remark42/app/common/static-store.ts 100.00% <ø> (ø)
frontend/apps/remark42/app/common/types.ts 100.00% <ø> (ø)
frontend/apps/remark42/app/utils/errorUtils.ts 84.61% <ø> (ø)
frontend/apps/remark42/app/common/api.ts 72.09% <66.66%> (ø)
...ribe-by-email/comment-form__subscribe-by-email.tsx 90.40% <93.75%> (-0.20%) ⬇️
frontend/apps/remark42/app/common/fetcher.ts 94.82% <100.00%> (ø)

... and 24 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@Jaskon
Copy link
Contributor Author

Jaskon commented Jun 25, 2023

#1645 changes are also here. They were required for this change and I was forced to merge that branch into this one.

@umputun umputun requested review from paskal and akellbl4 June 25, 2023 00:33
paskal
paskal previously approved these changes Jun 28, 2023
Copy link
Sponsor Collaborator

@paskal paskal left a comment

Choose a reason for hiding this comment

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

Here is what I get after trying to subscribe with the same email once I've already subscribed (but interface didn't update for some reason):

image

The core functionality is fine, but I wonder if we can handle this code 409 edge case differently so that interface will just update with "you've subscribed" instead of showing me the error before I reload the page.

prevStep is not used anywhere else and because of it influences output text (haveSubscribed), have changed it to more intuitive justSubscribed variable
@Jaskon
Copy link
Contributor Author

Jaskon commented Jun 28, 2023

@paskal Could you please review it again?
Added a handler for 409 http error.

@paskal
Copy link
Sponsor Collaborator

paskal commented Jun 29, 2023

Works like a charm! Please rebase and we'll merge it.

paskal
paskal previously approved these changes Jun 29, 2023
@umputun umputun merged commit 497f3ce into umputun:master Jun 29, 2023
9 checks passed
@paskal paskal added this to the v1.12.0 milestone Jul 23, 2023
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.

None yet

3 participants