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

[#3793] Change payment reference number #3827

Merged
merged 12 commits into from
Feb 12, 2024
Merged

[#3793] Change payment reference number #3827

merged 12 commits into from
Feb 12, 2024

Conversation

Viicos
Copy link
Contributor

@Viicos Viicos commented Jan 30, 2024

Fixes #3793

@Viicos
[#3793] Change payment reference number
cab6286 
The internal order ID isn't relevant anywore, instead we rely
on the PK of the model. The race condition handling is also
not relevant anymore as we rely on the PK which is already supposed
to be unique.
@Viicos Viicos force-pushed the feature/3793-payment-ref branch 4 times, most recently from 5c5cc4f to 5dfabd5 Compare January 30, 2024 13:37
@Viicos Viicos marked this pull request as ready for review January 30, 2024 14:26
@codecov Codecov
Copy link

codecov bot commented Jan 31, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (8a84d35) 96.33% compared to head (1aaa158) 96.34%.
Report is 7 commits behind head on master.

❗ Current head 1aaa158 differs from pull request most recent head 5af8e32. Consider uploading reports for the commit 5af8e32 to get more accurate results

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #3827      +/-   ##
==========================================
+ Coverage   96.33%   96.34%   +0.01%     
==========================================
  Files         707      707              
  Lines       22173    22163      -10     
  Branches     2541     2542       +1     
==========================================
- Hits        21360    21353       -7     
+ Misses        568      565       -3     
  Partials      245      245

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

vaszig
vaszig approved these changes Feb 2, 2024
View reviewed changes
Copy link
Contributor

@vaszig vaszig left a comment

Choose a reason for hiding this comment

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

Please add the steps to the internal taiga as we did with the other VCR recordings.

@Viicos
Copy link
Contributor Author

Viicos commented Feb 2, 2024

Please add the steps to the internal taiga as we did with the other VCR recordings.

Good catch

sergei-maertens
sergei-maertens requested changes Feb 2, 2024
View reviewed changes
Copy link
Member

@sergei-maertens sergei-maertens left a comment

Choose a reason for hiding this comment

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

Some fine-tuning, looks good otherwise!

@Viicos Viicos force-pushed the feature/3793-payment-ref branch 2 times, most recently from 0a40316 to dcb2fff Compare February 2, 2024 15:55
@Viicos Viicos force-pushed the feature/3793-payment-ref branch from 0d906dc to 8f6e3bd Compare February 5, 2024 13:39
@Viicos
Copy link
Contributor Author

Viicos commented Feb 5, 2024

Tests passed without 7c5de5c, isn't there any tests/checks to have admin fields actually present on the model?

@sergei-maertens
Copy link
Member

Tests passed without 7c5de5c, isn't there any tests/checks to have admin fields actually present on the model?

unfortunately not, since they can also be lazy/callables to a number of places (model methods, admin methods and perhaps even plain python callables isntead of string references)

sergei-maertens
sergei-maertens requested changes Feb 5, 2024
View reviewed changes
@Viicos Viicos force-pushed the feature/3793-payment-ref branch from d449b6d to 6b42e04 Compare February 8, 2024 13:10
@Viicos Viicos force-pushed the feature/3793-payment-ref branch from e5005ad to 7f9b02e Compare February 8, 2024 16:37
@Viicos Viicos force-pushed the feature/3793-payment-ref branch from 7f9b02e to 1aaa158 Compare February 8, 2024 16:37
@Viicos
[#3793] Update asgiref to 3.7.2
d85dc29 
Provides better type checking and wanted to make sure
the vendored `database_sync_to_async` was compatible
@Viicos
[#3793] Use database_sync_to_async to properly close db connections
5af8e32
sergei-maertens
sergei-maertens reviewed Feb 12, 2024
View reviewed changes
src/openforms/tests/e2e/database_sync_to_async.py
Comment on lines +39 to +42
class DatabaseSyncToAsync(SyncToAsync):
"""
SyncToAsync version that cleans up old database connections when it exits.
"""
Copy link
Member

Choose a reason for hiding this comment

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

looks familiar :) https://github.com/maykinmedia/zgw-consumers/blob/main/zgw_consumers/concurrent.py#L27

sergei-maertens
sergei-maertens approved these changes Feb 12, 2024
View reviewed changes
Copy link
Member

@sergei-maertens sergei-maertens left a comment

Choose a reason for hiding this comment

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

Let's hope this is the one 🤞

@Viicos Viicos merged commit 09f71d7 into master Feb 12, 2024
25 checks passed
@Viicos Viicos deleted the feature/3793-payment-ref branch February 12, 2024 14:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vaszig vaszig vaszig approved these changes

@sergei-maertens sergei-maertens sergei-maertens approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

As customer I want to see the form reference on the payment transaction
3 participants
@Viicos @sergei-maertens @vaszig