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

fix(snapshot): define resourceToPreview from resources changed. #1453

Merged
merged 6 commits into from
Mar 14, 2024

Conversation

louis-bompart
Copy link
Collaborator

@louis-bompart louis-bompart commented Mar 12, 2024

Proposed changes

The current algorithm doesn't take into account the resources the user can access.
To solves that, we could either:

  • Query/cache what resources the user can access when we query it
  • Only do the expanded preview on resources that change.

Breaking changes

Testing

  • Unit Tests:
    Snapshot UTs are tangled, changing it will be difficult/require a rewrite.
  • Functionnal Tests:
    Corner-case, would require to create orgs without some access right or some stuff like that.
  • Manual Tests:
    In the only known affected environment (customer-owned), I managed to reproduce the issue the same customer reported.
  • Tested with the current version of the CLI, dev build from master: Crash
  • Tested with dev build from this branch: No Crash.

https://coveord.atlassian.net/browse/CDX-1530

@louis-bompart louis-bompart requested a review from a team as a code owner March 12, 2024 23:09
@louis-bompart louis-bompart requested review from olamothe, y-lakhdar and fbeaudoincoveo and removed request for a team March 12, 2024 23:09
Copy link
Contributor

Thanks for your contribution @louis-bompart !
When your pull-request is ready to be merged, check the box below to merge it

  • Merge! :shipit:

Copy link
Contributor

Pull Request Report

PR Title

✅ Title follows the conventional commit spec.

@louis-bompart louis-bompart enabled auto-merge (squash) March 14, 2024 19:03
@louis-bompart louis-bompart merged commit c4095f8 into master Mar 14, 2024
48 checks passed
@louis-bompart louis-bompart deleted the snapshot-expanded-preview-hackfix branch March 14, 2024 21:04
louis-bompart added a commit that referenced this pull request Mar 18, 2024
<!-- For Coveo Employees only. Fill this section.

CDX-1534

-->

## Proposed changes

#1453 , I didn't test the suggestion, and it was breaking the code.
Iterating from the OG code + suggestions, here's a nice solution.

## Testing:
 - Reproduced reported issue w/ master and latest release
 - Did not with this change
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants