Skip to content

Conversation

@prateekj117
Copy link
Member

Fixes #6382

Checklist

  • I have read the Contribution & Best practices Guide and my PR follows them.
  • My branch is up-to-date with the Upstream development branch.
  • The unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • All the functions created/modified in this PR contain relevant docstrings.

@auto-label auto-label bot added the fix label Aug 17, 2019
Copy link
Member

@iamareebjamal iamareebjamal left a comment

Choose a reason for hiding this comment

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

Do you think after the previous migration has failed due to duplicate ticket tags, this will even get executed?

And do you think setting deleted_at will do anything to prevent the unique constraint fot ticket tags?

You are again sending the PR without reproducing the bug and even verifying that it works. Flailing your arms in a dark room blindfolded will not solve this bug

@prateekj117
Copy link
Member Author

prateekj117 commented Aug 17, 2019

@iamareebjamal I changed the revision and downrevision appropriately. And I am also appending '.deleted' to the ticket-tag.

It works locally. I check running the migrations. Though as one more migration has merged for event_invoice, I have to make those changes.

@codecov
Copy link

codecov bot commented Aug 17, 2019

Codecov Report

Merging #6386 into development will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##           development    #6386   +/-   ##
============================================
  Coverage        65.16%   65.16%           
============================================
  Files              287      287           
  Lines            14810    14810           
============================================
  Hits              9651     9651           
  Misses            5159     5159

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 ead0ba0...f244528. Read the comment docs.

@prateekj117
Copy link
Member Author

@iamareebjamal Done.

@iamareebjamal
Copy link
Member

This will work, but what about tickets linked with the deleted ticket tag?

@prateekj117
Copy link
Member Author

@iamareebjamal I couldn't find where it's saving the information about the ticket linked with the ticket-tag.

@iamareebjamal
Copy link
Member

Ran your migration. There are still duplicates and the proceeding migration failed. What did you even test?

@CosmicCoder96 Are ticket tags even being used, there is no entry in association table?

@prateekj117
Copy link
Member Author

prateekj117 commented Aug 17, 2019

@iamareebjamal I tested it using in and not not in because I had the unique constraint already.
Sorry for the Negligence.
Weirdly enough, it turns out not in is returning no entries even in case of duplicates.

@iamareebjamal
Copy link
Member

Then it is wrong query

@prateekj117
Copy link
Member Author

@iamareebjamal I have corrected the query. Please review.

@iamareebjamal
Copy link
Member

Doesn't work if there are more than 2 duplicates

@abhinavk96
Copy link
Contributor

abhinavk96 commented Aug 18, 2019

@CosmicCoder96 Are ticket tags even being used, there is no entry in association table?

@iamareebjamal I don't think they are consumed by current FE, but they are anyway supported by the ticketing API, referred to multiple times in it. And the only ticket tags I see are for some events on legacy data.
Association table is empty even inside legacy database.

@iamareebjamal
Copy link
Member

@prateekj117 Wrap this up ASAP, please

@prateekj117
Copy link
Member Author

@iamareebjamal It's working even when there are more than two duplicates:

Screenshot from 2019-08-18 22-05-56

@iamareebjamal
Copy link
Member

Yeah, so exactly @prateekj117

Do you think the unique constraint will work on the data above?

@iamareebjamal
Copy link
Member

(15, paid !!!!.deleted) is still duplicated. So how it is going to work?

@prateekj117
Copy link
Member Author

@iamareebjamal Ok, I got the point.

@prateekj117
Copy link
Member Author

@iamareebjamal Should I delete the rows?

@iamareebjamal
Copy link
Member

No

@prateekj117
Copy link
Member Author

@iamareebjamal Can you give me a rough idea on how to go about for a query like that?

@iamareebjamal
Copy link
Member

Append id or fixed random characters before .deleted

And change the implementation of downgrade accordingly

@prateekj117
Copy link
Member Author

@iamareebjamal Done.

@iamareebjamal
Copy link
Member

@CosmicCoder96 Please verify

Copy link
Contributor

@abhinavk96 abhinavk96 left a comment

Choose a reason for hiding this comment

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

Tested on production data locally, worked.

@iamareebjamal iamareebjamal merged commit 8808d7c into fossasia:development Aug 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Ticket tag migrations failing for production data

3 participants