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

Can't add new report actions in Debug mode #53683

Open
DylanDylann opened this issue Dec 6, 2024 · 16 comments
Open

Can't add new report actions in Debug mode #53683

DylanDylann opened this issue Dec 6, 2024 · 16 comments
Assignees
Labels
Reviewing Has a PR in review Weekly KSv2

Comments

@DylanDylann
Copy link
Contributor

DylanDylann commented Dec 6, 2024

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


Coming from #53027 (comment)

Debug.setDebugData(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportID}`, {[parsedReportAction.reportActionID]: parsedReportAction});

we use the set method but we don't add the previous data. It caused when adding new report action, all previous report actions will be removed

cc @pac-guerreiro @puneetlath @JKobrynski

Issue OwnerCurrent Issue Owner: @DylanDylann
@Shahidullah-Muffakir
Copy link
Contributor

Proposal

Please re-state the problem that we are trying to solve in this issue.

We are not keeping the old state in onyx

What is the root cause of that problem?

We are using set method of onyx, we should use merge

What changes do you think we should make in order to solve the problem?

here:

function setDebugData<TKey extends OnyxKey | `${OnyxCollectionKey}${string}`>(onyxKey: TKey, onyxValue: OnyxMergeInput<TKey>) {

we can create one method for merge as well as:

function mergeDebugData<TKey extends OnyxKey | `${OnyxCollectionKey}${string}`>(onyxKey: TKey, onyxValue: OnyxMergeInput<TKey>) {
    Onyx.merge(onyxKey, onyxValue);
}

and then use it

@melvin-bot melvin-bot bot added the Daily KSv2 label Dec 6, 2024
@DylanDylann
Copy link
Contributor Author

@pac-guerreiro or @JKobrynski will handle this issue. No require proposals

@JKobrynski
Copy link
Contributor

Hi, I'm Julian from Callstack - expert agency - and I would like to work on this issue.

@DylanDylann
Copy link
Contributor Author

@JKobrynski Feel free to raise a PR

@melvin-bot melvin-bot bot added the Overdue label Dec 11, 2024
@puneetlath
Copy link
Contributor

@JKobrynski do you have an ETA for a PR?

@pac-guerreiro
Copy link
Contributor

@DylanDylann @puneetlath I'm back from my vacation so I'm taking this from @JKobrynski 😄

I'll raise a PR at the end of my work day 😄

@melvin-bot melvin-bot bot removed the Overdue label Dec 11, 2024
@puneetlath
Copy link
Contributor

Welcome back!

@pac-guerreiro
Copy link
Contributor

@puneetlath thanks! 😄

@DylanDylann I just opened a draft PR - #53969

I'll need to add screen recordings tomorrow!

If the PR is too big, I can split it in two 😉

@puneetlath
Copy link
Contributor

Looks ok to me in terms of size, thanks!

@pac-guerreiro
Copy link
Contributor

Today:

  • Add testing steps
  • Add screen recordings

Todo:

  • Fix Not Found screen shown when navigating back after deleting a report

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Dec 12, 2024
@pac-guerreiro
Copy link
Contributor

pac-guerreiro commented Dec 12, 2024

@puneetlath @DylanDylann The PR is now ready for review! 😄

There's a minor issue where the Not Found page is shown if the user goes back after deleting a report. I'll look for a fix for this issue tomorrow!

@pac-guerreiro
Copy link
Contributor

@DylanDylann @puneetlath

Last friday (12/13/24)

  • I didn't have much time to focus on the remaining issue but I did some investigation and got some ideas to try

Today

  • I tried some ideas and managed to find one that fixed the Not Found page showing after deleting a report, going to concierge and pressing back button on the header

@pac-guerreiro
Copy link
Contributor

Waiting on @DylanDylann review 😄

@pac-guerreiro
Copy link
Contributor

@puneetlath

@DylanDylann approved the PR 🥳 Feel free to chime in and let me know if there's anything that needs my attention 😄

@pac-guerreiro
Copy link
Contributor

The PR was finally merged yesterday! 🥳

Copy link

melvin-bot bot commented Dec 24, 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Reviewing Has a PR in review Weekly KSv2
Projects
None yet
Development

No branches or pull requests

5 participants