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

VPN-2452 - Subscription Expiring Message #5273

Closed
wants to merge 4 commits into from
Closed

Conversation

jps-moz
Copy link
Contributor

@jps-moz jps-moz commented Dec 20, 2022

Description

-Implemented a message addon to notify users when their subscription is expiring.
-They also get an OS notification (until background notifications for mobile is implemented, this will only be for Desktop).
-The ‘Subscription expiring’ message doesn’t show anymore once the subscription end date > 7 days again.

Reference

https://mozilla-hub.atlassian.net/browse/VPN-2542

Checklist

  • My code follows the style guidelines for this project
  • I have not added any packages that contain high risk or unknown licenses (GPL, LGPL, MPL, etc. consult with DevOps if in question)
  • I have performed a self review of my own code
  • I have commented my code PARTICULARLY in hard to understand areas
  • I have added thorough tests where needed

@codecov-commenter
Copy link

codecov-commenter commented Dec 21, 2022

Codecov Report

Base: 31.40% // Head: 25.61% // Decreases project coverage by -5.79% ⚠️

Coverage data is based on head (4610e1a) compared to base (c3cd2a2).
Patch has no changes to coverable lines.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5273      +/-   ##
==========================================
- Coverage   31.40%   25.61%   -5.79%     
==========================================
  Files         353       12     -341     
  Lines       23821      605   -23216     
  Branches    12951      324   -12627     
==========================================
- Hits         7480      155    -7325     
+ Misses       9872      261    -9611     
+ Partials     6469      189    -6280     
Flag Coverage Δ
functional_tests ?
linux_unit_tests ?
lottie_tests 22.97% <ø> (ø)
macos_unit_tests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/apps/vpn/inspector/inspectorhandler.cpp
src/apps/vpn/models/servercity.h
src/apps/vpn/commands/commandactivate.cpp
src/shared/platforms/linux/linuxcryptosettings.cpp
src/shared/logger.cpp
src/apps/vpn/captiveportal/captiveportalrequest.h
src/apps/vpn/commands/commanddeactivate.cpp
src/apps/vpn/dnshelper.cpp
tests/unit/testcheckedint.cpp
src/shared/rfc/rfc5735.cpp
... and 331 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

Copy link
Collaborator

@bakulf bakulf left a comment

Choose a reason for hiding this comment

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

Add a functional test for this addon to simulate its visibility. See the addon folder in tests/tunctional

@jps-moz
Copy link
Contributor Author

jps-moz commented Dec 30, 2022

Add a functional test for this addon to simulate its visibility. See the addon folder in tests/tunctional

After exploring this folder and the surrounding test folders, I was unable to find a clear way to trigger the new message directly (for example, by editing the expiresOn property). It seems like these functional tests are for testing new types of addons or new functionality they have, which this doesn't fit IMO. From what I understand in this case, I would essentially be duplicating the code, but not testing the one actually seen by the user. Is there something I am misunderstanding here?

@brizental brizental force-pushed the VPN-2452-sub-expire branch from 94fde9f to 4610e1a Compare January 23, 2023 16:26
@brizental brizental requested a review from strseb as a code owner January 23, 2023 16:26
@brizental brizental closed this Jan 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.

5 participants