-
Notifications
You must be signed in to change notification settings - Fork 35
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
B21575-main3-add_order_types #14206
B21575-main3-add_order_types #14206
Conversation
Bundle StatsHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset
View detailed bundle breakdownAdded
Removed
Bigger No assets were bigger Smaller No assets were smaller Unchanged
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
found no diff between INT and MAIN PRs. lgtm 👽
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like there are some linter fixes needed in couple of these files. Other than that - LGTM
Agreed. I think the Happo is showing something broken for the File Viewer or something too.. at least 1 part of this one may get resolved by updating the branch. |
I believe I've resolved those. Please re-evaluate. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happo diffs look good. However, this commit 934f477 is not in integration.
Please link all integration PRs that this main PR is covering. The only int pr linked covers just... wait. the commits between the linked PR and this PR don't match up? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay I'm definitely confused on what's going on with this PR, there's some substantial differences between this one and the INT ones. Commits don't line up, and changes aren't the exact same either.
I hate to throw a wrench into this, but it's important things match between environments. See review comments for more info and let me know if there's anything I can do to help get things back on track.
Edit: After some discussion, the only remaining issue is the following commit: 934f477
@@ -13,7 +13,7 @@ import ToolTip from 'shared/ToolTip/ToolTip'; | |||
import { DatePickerInput, DropdownInput, DutyLocationInput } from 'components/form/fields'; | |||
import { Form } from 'components/form/Form'; | |||
import SectionWrapper from 'components/Customer/SectionWrapper'; | |||
import { ORDERS_PAY_GRADE_OPTIONS } from 'constants/orders'; | |||
import { ORDERS_PAY_GRADE_OPTIONS, ORDERS_TYPE } from 'constants/orders'; | |||
import { dropdownInputOptions } from 'utils/formatters'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file is not edited on int pr https://github.com/transcom/mymove/pull/14211/files or https://github.com/transcom/mymove/pull/14378/files, or even https://github.com/transcom/mymove/pull/14164/files
Trying to see if I can help, but I've created a new branch off the IntergrationBranch just now and merged in this branch into it to see. This is the PR. I only see 1 diff which is the |
This is very helpful as it lets us see that all of the discrepancies I listed above (aside from # 4, that's the additional commit added that you mention) are actually in integrationTesting, even if it's confusing how exactly that happened. Seems like maybe we've lost an int pr somewhere along the way? Either way, it implies that this should be a much more solid PR once 934f477 is added to integration first. |
I think this will help clarify why things seem so odd.
|
Yes, or deleted from this PR because it doesn't look like it belong in this BL. |
That'll do it. I'm sorry that you've had to deal with so many conflicts, especially after dev was completed. That makes organizing by dependency much more difficult and harder to follow. In that case, never mind any of the other differences. It'd be an exercise in futility to track them all down if they're the result of conflicts like that. All of the changes match except for 934f477, then this will be g2g without that commit like @taeJungCaci suggests, or once the commit is pushed to main. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Requesting change as mentioned above
934f477
to
b0b4d9c
Compare
Removed the "934f477" commit. |
Agility ticket
B21575
INT PR
14211
Summary/Testing/ANDI/Screenshots