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

[PM-7413] fix(8560): refreshing reports pages displays empty pages #8700

Merged

Conversation

abarghoud
Copy link
Contributor

@abarghoud abarghoud commented Apr 11, 2024

Type of change

- [X] Bug fix
- [ ] New feature development
- [ ] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other

Objective

Resolved #8560, ciphers data are lost when refreshing reports pages

Code changes

  • cipher-report.component.ts: Adds a call to syncService.fullSync(false) in the load() method
  • admin-console/organizations/tools/*-report.component.ts: addition of syncService dependency
  • tools/reports/pages/*-report.component.ts: addition of syncService dependency
  • *.spec.ts: addition of a test to make sure syncService.fullSync(false) is called

Before you submit

  • Please add unit tests where it makes sense to do so (encouraged but not required)
  • If this change requires a documentation update - notify the documentation team
  • If this change has particular deployment requirements - notify the DevOps team
  • Ensure that all UI additions follow WCAG AA requirements

@abarghoud abarghoud requested review from a team as code owners April 11, 2024 16:08
@abarghoud abarghoud requested a review from r-tome April 11, 2024 16:08
@bitwarden-bot
Copy link

Thank you for your contribution! We've added this to our internal Community PR board for review.
ID: PM-7413

@bitwarden-bot bitwarden-bot changed the title fix(8560): refreshing reports pages displays empty pages [PM-7413] fix(8560): refreshing reports pages displays empty pages Apr 11, 2024
@abarghoud
Copy link
Contributor Author

abarghoud commented Apr 11, 2024

I fixed it this way, but I think it could be resolved differently with resolvers, defined at the routes level, which blocks access to the page till all necessary data is fetched; I did not see it implemented elsewhere in the repo.

@eliykat
Copy link
Member

eliykat commented Apr 18, 2024

The main operational change here is to cipher-report.component.ts which is owned by @bitwarden/team-tools-dev so I'll defer to them on the review. If that is approved then the AC Team changes are uncontroversial.

aj-rosado
aj-rosado previously approved these changes Apr 23, 2024
Copy link
Contributor

@aj-rosado aj-rosado left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your contribution @abarghoud !

This change seems to fix the issue, I'm passing it to QA testing.

r-tome
r-tome previously approved these changes Apr 23, 2024
@aj-rosado
Copy link
Contributor

@abarghoud there are some changes on the reports files that are conflicting with the changes on this PR.
Can you merge this branch with the most recent version from main and resolve the conflicts please?

If you can't, could you give me permission to push into your repository so that I can fix them?

thank you!

# Conflicts:
#	apps/web/src/app/admin-console/organizations/tools/exposed-passwords-report.component.ts
#	apps/web/src/app/admin-console/organizations/tools/inactive-two-factor-report.component.ts
#	apps/web/src/app/admin-console/organizations/tools/reused-passwords-report.component.ts
#	apps/web/src/app/admin-console/organizations/tools/unsecured-websites-report.component.ts
#	apps/web/src/app/admin-console/organizations/tools/weak-passwords-report.component.ts
#	apps/web/src/app/tools/reports/pages/cipher-report.component.ts
#	apps/web/src/app/tools/reports/pages/exposed-passwords-report.component.spec.ts
#	apps/web/src/app/tools/reports/pages/exposed-passwords-report.component.ts
#	apps/web/src/app/tools/reports/pages/inactive-two-factor-report.component.spec.ts
#	apps/web/src/app/tools/reports/pages/inactive-two-factor-report.component.ts
#	apps/web/src/app/tools/reports/pages/reused-passwords-report.component.spec.ts
#	apps/web/src/app/tools/reports/pages/reused-passwords-report.component.ts
#	apps/web/src/app/tools/reports/pages/unsecured-websites-report.component.spec.ts
#	apps/web/src/app/tools/reports/pages/unsecured-websites-report.component.ts
#	apps/web/src/app/tools/reports/pages/weak-passwords-report.component.spec.ts
#	apps/web/src/app/tools/reports/pages/weak-passwords-report.component.ts
@abarghoud abarghoud dismissed stale reviews from r-tome and aj-rosado via 3f7f90d May 2, 2024 13:21
@abarghoud
Copy link
Contributor Author

@abarghoud there are some changes on the reports files that are conflicting with the changes on this PR. Can you merge this branch with the most recent version from main and resolve the conflicts please?

If you can't, could you give me permission to push into your repository so that I can fix them?

thank you!

@aj-rosado Done ;)

@aj-rosado aj-rosado merged commit ed236df into bitwarden:main May 3, 2024
21 of 26 checks passed
@aj-rosado
Copy link
Contributor

Thank you for your contribution @abarghoud ! QA has approved it and has been merged into main!

This will be part of the next release ! 🎉

@abarghoud abarghoud deleted the 8560-refreshing-does-not-retrieve-data branch May 3, 2024 19:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refreshing any reports pages leads to the displaying an empty state
5 participants