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

[HOLD for payment 2024-03-19] [Violations] Implement violations for mutiple tag lists #37117

Closed
cead22 opened this issue Feb 23, 2024 · 11 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Engineering Improvement Item broken or needs improvement. Reviewing Has a PR in review Weekly KSv2

Comments

@cead22
Copy link
Contributor

cead22 commented Feb 23, 2024

  • When adding support for multiple tags (PR), we broke some violation functionality for single-level tags, and afaict didn't implement them correctly for multi-level tags
  • One reason I think we didn't implement them correctly, and this is something I also missed in the original design, is that the Missing $tagLisName (eg, Missing Department) violation, isn't represented with a missingTag violation, but with a someTagLevelsRequired with a data object containing errorIndexes for the tags that are missing. I didn't see any functionality around that in the PR linked above
  • I was initially going to add the fix for that in this PR, but once I found about ^ I decided to fix single level tags and update the function signatures I set out to update in that PR, and implement the multi tag violations in a separate PR (which will be linked to this issue)

Multi tag file for tests multitags.csv

Copy link

melvin-bot bot commented Feb 26, 2024

@cead22 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@cead22
Copy link
Contributor Author

cead22 commented Feb 27, 2024

WIP. I'll have a PR up this week

@cead22
Copy link
Contributor Author

cead22 commented Feb 29, 2024

Draft PR is up

@cead22
Copy link
Contributor Author

cead22 commented Mar 2, 2024

PR updated, and should be ready for review early next week

@melvin-bot melvin-bot bot added Overdue Reviewing Has a PR in review Weekly KSv2 and removed Overdue Daily KSv2 labels Mar 4, 2024
@cead22
Copy link
Contributor Author

cead22 commented Mar 5, 2024

PR submitted for review

Copy link

melvin-bot bot commented Mar 11, 2024

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

@JmillsExpensify JmillsExpensify changed the title Implement violations for mutiple tag lists [Violations] Implement violations for mutiple tag lists Mar 12, 2024
@cead22
Copy link
Contributor Author

cead22 commented Mar 12, 2024

This was done but surfaced another bug #38044, and I found the issue. Let's continue the work there

@cead22 cead22 closed this as completed Mar 12, 2024
@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Mar 12, 2024
@melvin-bot melvin-bot bot changed the title [Violations] Implement violations for mutiple tag lists [HOLD for payment 2024-03-19] [Violations] Implement violations for mutiple tag lists Mar 12, 2024
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Mar 12, 2024
Copy link

melvin-bot bot commented Mar 12, 2024

Reviewing label has been removed, please complete the "BugZero Checklist".

Copy link

melvin-bot bot commented Mar 12, 2024

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.50-5 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2024-03-19. 🎊

@cead22
Copy link
Contributor Author

cead22 commented Jun 18, 2024

Dependent_tags_tags.csv

@cead22
Copy link
Contributor Author

cead22 commented Jun 25, 2024

popbagel.pdf

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Engineering Improvement Item broken or needs improvement. Reviewing Has a PR in review Weekly KSv2
Projects
Status: Done
Development

No branches or pull requests

1 participant