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

[PS 1885] Fixing error when exporting an organization vault #4367

Closed

Conversation

cyprain-okeke
Copy link
Contributor

Type of change

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

Objective

Fixing an error is displayed when exporting an organization vault

Code changes

  • libs/common/src/models/response/organization-export.response.ts: map the data array inside the collection object and cipher object

Screenshots

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

@cyprain-okeke cyprain-okeke requested a review from a team January 2, 2023 09:54
Copy link
Contributor

@djsmith85 djsmith85 left a comment

Choose a reason for hiding this comment

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

@cyprain-okeke Changes are looking good and address the issue with the broken export.

Just some minor improvements.

Please also speak with @r-tome as this was introduced with (EC-584) and passed QA.

@@ -9,12 +9,12 @@ export class OrganizationExportResponse extends BaseResponse {
constructor(response: any) {
super(response);
const collections = this.getResponseProperty("Collections");
if (collections != null) {
this.collections = collections.map((c: any) => new CollectionResponse(c));
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we add a type here? I think we are still trying to receive a ListResponse<T>(ListResponse<CollectionResponse>?) from this.getResponseProperty("Collections")
This would have shown the type error during development.

}
const ciphers = this.getResponseProperty("Ciphers");
if (ciphers != null) {
this.ciphers = ciphers.map((c: any) => new CipherResponse(c));
Copy link
Contributor

Choose a reason for hiding this comment

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

See comment above.

@@ -9,12 +9,12 @@ export class OrganizationExportResponse extends BaseResponse {
constructor(response: any) {
super(response);
const collections = this.getResponseProperty("Collections");
if (collections != null) {
this.collections = collections.map((c: any) => new CollectionResponse(c));
if (collections.data != null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The null checks should check both objects (collections and collections.data)

@r-tome
Copy link
Contributor

r-tome commented Jan 3, 2023

@cyprain-okeke Changes are looking good and address the issue with the broken export.

Just some minor improvements.

Please also speak with @r-tome as this was introduced with (EC-584) and passed QA.

This issue has already been fixed. Client versions from 2022.09 until 2022.12 expect an object with ListResponse properties but, as @djsmith85 pointed out, that has been reverted #3641.

Now server checks for the client version and responds with what the client expects (ListResponse or an array) https://github.com/bitwarden/server/blob/master/src/Api/Controllers/OrganizationExportController.cs#L44

QA tested both client versions hence why it passed the tests.
@cyprain-okeke let me know if you need help testing this.

@cyprain-okeke
Copy link
Contributor Author

I have tested the fix on version 2023.1 and it works very well.

@cyprain-okeke cyprain-okeke deleted the ps-1885/an-error-is-displayed-when-exporting branch January 3, 2023 15:36
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.

4 participants