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

[#expense] [Bring copilot to NewDot] Re-add the Try New Expensify button (when applicable to show it) within HybridApp for Copilot #51264

Open
dangrous opened this issue Oct 22, 2024 · 30 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Reviewing Has a PR in review

Comments

@dangrous
Copy link
Contributor

dangrous commented Oct 22, 2024

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


E/A issue for https://github.com/Expensify/Expensify/issues/414571 . We removed this functionality before copilot was migrated over, and now it is! @war-in is working on PRs already.

View all open jobs on GitHub

@dangrous dangrous added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Oct 22, 2024
@dangrous dangrous self-assigned this Oct 22, 2024
Copy link

melvin-bot bot commented Oct 22, 2024

Triggered auto assignment to @garrettmknight (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

@garrettmknight garrettmknight changed the title [#wave-control] [Bring copilot to NewDot] Re-add the Try New Expensify button (when applicable to show it) within HybridApp for Copilot [#expense] [Bring copilot to NewDot] Re-add the Try New Expensify button (when applicable to show it) within HybridApp for Copilot Oct 22, 2024
@war-in
Copy link
Contributor

war-in commented Oct 23, 2024

Hi 👋 I'm working on PRs here (ND part, OD part)

@dangrous
Copy link
Contributor Author

assigned!

@dangrous
Copy link
Contributor Author

@war-in if they're ready, can you put the PRs into review? You can add a [HOLD] to the title so they won't be merged early

@melvin-bot melvin-bot bot added the Overdue label Oct 28, 2024
@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 Overdue labels Oct 28, 2024
@war-in
Copy link
Contributor

war-in commented Oct 28, 2024

@war-in if they're ready, can you put the PRs into review? You can add a [HOLD] to the title so they won't be merged early

PRs are marked as ready for review :) Mentioned you in both 🫡

@dangrous
Copy link
Contributor Author

@Ollyws you were assigned to review the App PR, let us know if you can take a look soon, thanks!

@war-in
Copy link
Contributor

war-in commented Nov 6, 2024

Hi 👋 I updated both PRs with the newest main and I'm waiting for the final review

@Ollyws
Copy link
Contributor

Ollyws commented Nov 6, 2024

Will review this one asap.

@Julesssss Julesssss assigned Julesssss and unassigned Ollyws and dangrous Nov 12, 2024
@Julesssss
Copy link
Contributor

This can't be tested by External contributors, so I'll unassign you @Ollyws.

Also, @dangrous I can take over reviewing and testing of the PRs

@Julesssss
Copy link
Contributor

Alright, next step is to wait for the App changes to hit prod then I'll bump the HybridApp submodule (like this) and test @war-in's HybridApp changes.

Back in a few days.

@Julesssss Julesssss removed the Reviewing Has a PR in review label Nov 12, 2024
@dangrous
Copy link
Contributor Author

Thanks @Julesssss !

@dangrous
Copy link
Contributor Author

@war-in do you want to give a summary of that other bug with the tokens you were encountering here, so @Julesssss can weigh in? So for some reason disconnecting as a delegate calls CreateLogin which then fails here because it's sending chat-expensify-com as the partner name. I think we could probably just add that partner name to the conditional there to fix it, but I'm not sure if that would cause other side effects. I'm happy to keep working on that side of things, but would love your input!

@Julesssss
Copy link
Contributor

Julesssss commented Nov 13, 2024

So for some reason disconnecting as a delegate calls CreateLogin which then fails here because it's sending chat-expensify-com as the partner name.

Oh thanks for sharing, was that discussion on another issue or something? Adding chat-Expensify-com as a partner seems reasonable to me; maybe we should just ask in Slack to see if anyone sees a problem with it. But yeah it would be great if you can continue with that and I'll handle the HybridApp reviews and tests 👍

@war-in
Copy link
Contributor

war-in commented Nov 13, 2024

Oh thanks for sharing, was that discussion on another issue or something?

I DMed @dangrous to check if it's a known issue or not and forgot to write a summary here, sorry!

@dangrous
Copy link
Contributor Author

Great, discussing in Slack here

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Nov 14, 2024
@melvin-bot melvin-bot bot changed the title [#expense] [Bring copilot to NewDot] Re-add the Try New Expensify button (when applicable to show it) within HybridApp for Copilot [HOLD for payment 2024-11-21] [#expense] [Bring copilot to NewDot] Re-add the Try New Expensify button (when applicable to show it) within HybridApp for Copilot Nov 14, 2024
Copy link

melvin-bot bot commented Nov 14, 2024

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.61-3 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-11-21. 🎊

For reference, here are some details about the assignees on this issue:

  • @war-in does not require payment (Contractor)

Copy link

melvin-bot bot commented Nov 14, 2024

@dangrous / @Julesssss @garrettmknight @war-in The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the BugZero Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button]

@garrettmknight garrettmknight moved this from Bugs and Follow Up Issues to Hold for Payment in [#whatsnext] #expense Nov 14, 2024
@war-in
Copy link
Contributor

war-in commented Nov 14, 2024

FYI, there's also the second PR (on the OD side) so let's not close the issue yet

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Nov 20, 2024
Copy link

melvin-bot bot commented Nov 21, 2024

Skipping the payment summary for this issue since all the assignees are employees or vendors. If this is incorrect, please manually add the payment summary SO.

@garrettmknight garrettmknight changed the title [HOLD for payment 2024-11-21] [#expense] [Bring copilot to NewDot] Re-add the Try New Expensify button (when applicable to show it) within HybridApp for Copilot [#expense] [Bring copilot to NewDot] Re-add the Try New Expensify button (when applicable to show it) within HybridApp for Copilot Nov 21, 2024
@melvin-bot melvin-bot bot added the Overdue label Nov 22, 2024
@dangrous
Copy link
Contributor Author

Not overdue, working on existing PRs, and I'll hopefully be pushing up 1-2 PRs today to fix the token issue for Hybrid App

Copy link

melvin-bot bot commented Nov 25, 2024

@garrettmknight, @dangrous, @Julesssss, @war-in Huh... This is 4 days overdue. Who can take care of this?

@garrettmknight garrettmknight added Reviewing Has a PR in review and removed Awaiting Payment Auto-added when associated PR is deployed to production labels Nov 25, 2024
@melvin-bot melvin-bot bot removed the Overdue label Nov 25, 2024
Copy link

melvin-bot bot commented Dec 3, 2024

@garrettmknight, @dangrous, @Julesssss, @war-in Whoops! This issue is 2 days overdue. Let's get this updated quick!

@Julesssss
Copy link
Contributor

Multiple PRs ongoing.

Copy link

melvin-bot bot commented Dec 5, 2024

@garrettmknight, @dangrous, @Julesssss, @war-in Eep! 4 days overdue now. Issues have feelings too...

@dangrous
Copy link
Contributor Author

dangrous commented Dec 5, 2024

Not overdue, the PRs that allow this to move forward are in review and then waiting on the one in review. The App PR is good to go with current backend, and now being tested with the changes. We'll be there soon!

Copy link

melvin-bot bot commented Dec 9, 2024

@garrettmknight, @dangrous, @Julesssss, @war-in 8 days overdue is a lot. Should this be a Weekly issue? If so, feel free to change it!

@dangrous
Copy link
Contributor Author

dangrous commented Dec 9, 2024

Working on one minor fix in the backend PR and then can move forward.

@dangrous
Copy link
Contributor Author

nearly there!

@dangrous
Copy link
Contributor Author

App is deployed - the backend PR is not yet merged but I think we won't need it to move forward with testing this and getting it done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Reviewing Has a PR in review
Projects
Status: Hold for Payment
Development

No branches or pull requests

5 participants