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

Release changes using launch darkly plus minor tweaks #1052

Merged
merged 13 commits into from
Dec 15, 2022

Conversation

seeker25
Copy link
Collaborator

@seeker25 seeker25 commented Dec 14, 2022

We're wanting to have three changes under feature flags. Two for CSO directly. One for the new PAD invoice refund statuses.

Also changes to the reverse receipt process.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the sbc-pay license (Apache 2.0).

@codecov
Copy link

codecov bot commented Dec 14, 2022

Codecov Report

Merging #1052 (90cb290) into main (b197b36) will increase coverage by 12.37%.
The diff coverage is 90.80%.

@@             Coverage Diff             @@
##             main    #1052       +/-   ##
===========================================
+ Coverage   79.31%   91.68%   +12.37%     
===========================================
  Files          21      162      +141     
  Lines        1426    10558     +9132     
===========================================
+ Hits         1131     9680     +8549     
- Misses        295      878      +583     
Flag Coverage Δ
bcolapi ∅ <ø> (?)
eventlistenerqueue 81.63% <ø> (?)
payadmin ∅ <ø> (?)
payapi 93.97% <86.79%> (?)
paymentreconciliationsqueue 91.32% <ø> (?)

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

Impacted Files Coverage Δ
pay-api/src/pay_api/services/__init__.py 100.00% <ø> (ø)
...ices/fas/routing_slip_status_transition_service.py 100.00% <ø> (ø)
...y-api/src/pay_api/services/internal_pay_service.py 90.00% <50.00%> (ø)
pay-api/src/pay_api/services/flags.py 84.74% <62.50%> (ø)
pay-api/src/pay_api/services/cfs_service.py 69.93% <80.00%> (ø)
pay-api/src/pay_api/services/fas/routing_slip.py 76.42% <84.61%> (ø)
jobs/payment-jobs/tasks/routing_slip_task.py 90.53% <97.05%> (ø)
pay-api/src/pay_api/__init__.py 89.65% <100.00%> (ø)
pay-api/src/pay_api/models/db.py 100.00% <100.00%> (ø)
pay-api/src/pay_api/schemas/utils.py 85.71% <100.00%> (ø)
... and 148 more

@seeker25 seeker25 added enhancement New feature or request maintenance labels Dec 14, 2022
@@ -2,6 +2,9 @@
"flagValues": {
"string-flag": "a string value",
"bool-flag": true,
"integer-flag": 10
"integer-flag": 10,
"NEW_REFUNDED_STATUSES": true,
Copy link
Collaborator Author

@seeker25 seeker25 Dec 14, 2022

Choose a reason for hiding this comment

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

in PROD (will need to setup):

NEW_REFUNDED_STATUSES = false  (Might need to wait a month)
CSO_BCOL_FIX = false (January to enable)
BAD_CSO_SERVICE_FEE = true (January to disable)

@@ -41,6 +41,7 @@
def create_app(run_mode=os.getenv('FLASK_ENV', 'production')):
"""Return a configured Flask App using the Factory method."""
app = Flask(__name__)
app.env = run_mode
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Had to add this, otherwise in test mode it wouldn't read from the config file.

Copy link
Collaborator

Choose a reason for hiding this comment

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

that's weird. Was it not getting any values or just not setting the test config version of them?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh was it just not getting ff values?

Copy link
Collaborator Author

@seeker25 seeker25 Dec 14, 2022

Choose a reason for hiding this comment

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

I'd run the unit tests in the debugger, would always be set to PRODUCTION strangely even though it was set as something different in the app.configuration

@classmethod
def _build_reversal_comment(cls, operation: ReverseOperation):
"""Build the comment for the reversal."""
return {
Copy link
Collaborator Author

@seeker25 seeker25 Dec 14, 2022

Choose a reason for hiding this comment

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

Put this in to make it a bit more robust. Going to confirm with CAS.
Top two exist already.

For Correction, the idea was to increment cas_version_suffix, and append it to a routing slip number if it's > 1

EX.

234054347, cas_version_suffix incremented becomes 234054347R2... 234054347R3 etc..

@seeker25 seeker25 changed the title Release changes plus minor tweaks Release changes using launch darkly plus minor tweaks Dec 14, 2022
@@ -151,13 +151,14 @@ def _release_payment(invoice: Invoice):

@staticmethod
def _refund_and_create_credit_memo(invoice: InvoiceModel):
from pay_api.services import flags # pylint: disable=import-outside-toplevel
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If I didn't put the import here, ran into circular dependency troubles or flags not having is_on

@@ -49,6 +49,7 @@ def get_payment_system_code(self):
def create_invoice(self, payment_account: PaymentAccount, # pylint: disable=too-many-locals
line_items: [PaymentLineItem], invoice: Invoice, **kwargs) -> InvoiceReference:
"""Create Invoice in PayBC."""
from pay_api.services import flags # pylint: disable=import-outside-toplevel
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If I didn't put the import here, ran into circular dependency troubles or flags not having is_on

@@ -284,6 +284,7 @@ def find_by_corp_type_and_filing_type( # pylint: disable=too-many-arguments
**kwargs
):
"""Calculate fees for the filing by using the arguments."""
from pay_api.services import flags # pylint: disable=import-outside-toplevel
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If I didn't put the import here, ran into circular dependency troubles or flags not having is_on

Copy link
Collaborator

@kialj876 kialj876 left a comment

Choose a reason for hiding this comment

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

Looks right to me

@seeker25
Copy link
Collaborator Author

Deploying this to DEV, just to do a smoke test.

@@ -59,12 +60,6 @@ def init_app(self, app):
client = ldclient_get()

app.extensions['featureflags'] = client
app.teardown_appcontext(self.teardown)

def teardown(self, exception): # pylint: disable=unused-argument; flask method signature
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This teardown needed to be removed, otherwise flags get stuck being cached. Everytime it evaluates it hits this warning:

image

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
2.1% 2.1% Duplication

@seeker25 seeker25 merged commit 34c9f4b into bcgov:main Dec 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants