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

Changes to duplicate handling #37

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

chadgates
Copy link
Contributor

Function to identify for earlier duplicate changed to check for status "S" or "P".
Adding new setting ERROR_ON_DUPLICATE with default value True to preserve how application behaves now. If False, duplicate messages will be handled like success messages, but message_id to still have _duplicate added to it.

Currently, if no error is created for duplicate, the content of the message is stored once more. So if this should not happen, then various options should be checked:
pyas2lib would have to be adjusted to "continue on duplicate" and let django-pyas2 decide what it wants to do with a duplicate message - maybe by adding another status "D" that could then be used to decide if file should be stored or not.
Or have pyas2lib receive options what to do on duplicate.

…se, duplicate messages will be handled like success messages, but message_id to still have _duplicate added to it.
@codecov
Copy link

codecov bot commented Jun 6, 2020

Codecov Report

Merging #37 into master will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #37      +/-   ##
==========================================
+ Coverage   97.56%   97.60%   +0.03%     
==========================================
  Files          11       11              
  Lines         576      584       +8     
==========================================
+ Hits          562      570       +8     
  Misses         14       14              
Impacted Files Coverage Δ
pyas2/settings.py 100.00% <100.00%> (ø)
pyas2/views.py 97.94% <100.00%> (+0.10%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9b8fac3...7438a52. Read the comment docs.

@abhishek-ram
Copy link
Owner

I think we need to remove the unique together for message_id and partner. If not when there is a failed message and then its retries it would cause integrity error i think, can you also add this to the test case.

@chadgates
Copy link
Contributor Author

That can be removed, but should we then also remove the "_duplicate" from the message id on receiving duplicates? This currently prevents the integrity error for the first retry.

@chadgates
Copy link
Contributor Author

Any changes requested for this PR?
Options are :

  • as per the PR, the "_duplicate" is kept in the message id and trailed by a unique value
    or
  • remove the "unique together" and remove the "_duplicate" from the file names - if this is the case, should it somehow still be visible that a transmission was a duplicate ? A status field maybe ?

@abhishek-ram
Copy link
Owner

So I guess I was wrong and it wont fail because I am doing an update or create in create_from_as2message method. But in essence it would update the existing message (case where flag is set to True and original message failed say due to invalid signature) i guess which is okay?

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.

2 participants