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.

Sign up for GitHub

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

Jump to bottom

Warn admin users if HTTPS is not required for checkout #1231

Merged
merged 4 commits into from
Jan 22, 2021
Merged

Warn admin users if HTTPS is not required for checkout #1231

merged 4 commits into from
Jan 22, 2021

Conversation

dmvrtx
Copy link
Contributor

@dmvrtx dmvrtx commented Jan 21, 2021 •
edited
Loading

Fixes #824

Changes proposed in this Pull Request

  • On plugin activation check if user has enabled HTTPS for their store or have forced SSL for checkout pages. If both conditions are not met create a WooCommerce notification urging them to force SSL for checkout pages.

Testing instructions

  • Bootstrap WooCommerce installation on a HTTP site
  • Open WooCommerce home page
  • There should be a notification like this:

image

  • Bootstrap WooCommerce installation on a HTTPS site
  • Open a WooCommerce home page
  • Notification "Force secure checkout" should not be there
  • Added changelog entry (or does not apply)
  • Covered with tests (or have a good reason not to test in description ☝️)
  • Tested on mobile (or does not apply)

dmvrtx added 2 commits January 21, 2021 15:51
@dmvrtx
Warn admin users if HTTPS is not required for checkout
76a5a80 
New notification class to notify customers that they might want to
enforce SSL on their WooCommerce store if HTTPS is not enabled globally
yet.

Addresses issue #824
@dmvrtx
Warn admin users if HTTPS is not required for checkout
0884a69 
Enable notification during plugin activation and introduce test suite.
@dmvrtx dmvrtx changed the title Add/checkout https notification Warn admin users if HTTPS is not required for checkout Jan 22, 2021
@kalessil
Copy link
Contributor

We need to add a new changelog entry in changelog.txt and readme.txt.

@kalessil kalessil self-requested a review January 22, 2021 09:39
kalessil
kalessil requested changes Jan 22, 2021
View reviewed changes
Copy link
Contributor

@kalessil kalessil left a comment

Choose a reason for hiding this comment

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

Overall looks great, left a couple of comments we need to address before merging.

includes/notes/class-wc-payments-notes-set-https-for-checkout.php Show resolved Hide resolved
@dmvrtx
Copy link
Contributor Author

dmvrtx commented Jan 22, 2021

We need to add a new changelog entry in changelog.txt and readme.txt.

Done.

kalessil
kalessil approved these changes Jan 22, 2021
View reviewed changes
Copy link
Contributor

@kalessil kalessil left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Pre approving, the guard condition changes is the last thing to address.

@dmvrtx
Move guard condition into can_be_added method to separate it from note
6ef914a 
`can_be_added` is supposed to be an override, but traits do not allow it
to be that simple. Thus it is imported from `NoteTraits` as a protected
method with a different name to be called from our `can_be_added` when
needed.
@kalessil kalessil added this to the Sprint 24 milestone Jan 22, 2021
@kalessil kalessil merged commit aa67429 into trunk Jan 22, 2021
@kalessil kalessil deleted the add/checkout-https-notification branch January 22, 2021 14:25
@csmcneill csmcneill mentioned this pull request May 25, 2024
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kalessil kalessil kalessil approved these changes

Assignees

@dmvrtx dmvrtx

Labels
pr: ready to merge
Projects
None yet
Milestone
Sprint 24
Development

Successfully merging this pull request may close these issues.

Warn admin users if HTTPS is not required for checkout
2 participants
@dmvrtx @kalessil