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(wallet): fix request export order #449

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,6 @@ const startDownload = async (idx: number): Promise<void> => {
} while (offset !== undefined);

const requestList = Array.from(requests.values());
requestList.sort((a, b) => a.request.created_at.localeCompare(b.request.created_at));

const csv = mapRequestsToCsvTable(downloadItem.group, requestList);
const fileName =
Expand Down
2 changes: 2 additions & 0 deletions apps/wallet/src/generated/station/station.did
Original file line number Diff line number Diff line change
Expand Up @@ -1385,6 +1385,8 @@ type GetNextApprovableRequestInput = record {
operation_types : opt vec ListRequestsOperationType;
// Exclude requests the user indicated to skip.
excluded_request_ids : vec UUID;
// Get the next request from a list sorted by the given field.
sort_by : opt ListRequestsSortBy;
};

// Result type for retrieving a request.
Expand Down
1 change: 1 addition & 0 deletions apps/wallet/src/generated/station/station.did.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -625,6 +625,7 @@ export type GetExternalCanisterResult = {
} |
{ 'Err' : Error };
export interface GetNextApprovableRequestInput {
'sort_by' : [] | [ListRequestsSortBy],
'excluded_request_ids' : Array<UUID>,
'operation_types' : [] | [Array<ListRequestsOperationType>],
}
Expand Down
13 changes: 7 additions & 6 deletions apps/wallet/src/generated/station/station.did.js
Original file line number Diff line number Diff line change
Expand Up @@ -1061,6 +1061,12 @@ export const idlFactory = ({ IDL }) => {
}),
'Err' : Error,
});
const SortByDirection = IDL.Variant({ 'Asc' : IDL.Null, 'Desc' : IDL.Null });
const ListRequestsSortBy = IDL.Variant({
'ExpirationDt' : SortByDirection,
'LastModificationDt' : SortByDirection,
'CreatedAt' : SortByDirection,
});
const ListRequestsOperationType = IDL.Variant({
'RemoveAsset' : IDL.Null,
'AddUserGroup' : IDL.Null,
Expand Down Expand Up @@ -1093,6 +1099,7 @@ export const idlFactory = ({ IDL }) => {
'AddAccount' : IDL.Null,
});
const GetNextApprovableRequestInput = IDL.Record({
'sort_by' : IDL.Opt(ListRequestsSortBy),
'excluded_request_ids' : IDL.Vec(UUID),
'operation_types' : IDL.Opt(IDL.Vec(ListRequestsOperationType)),
});
Expand Down Expand Up @@ -1276,7 +1283,6 @@ export const idlFactory = ({ IDL }) => {
}),
'Err' : Error,
});
const SortByDirection = IDL.Variant({ 'Asc' : IDL.Null, 'Desc' : IDL.Null });
const ListExternalCanistersSortInput = IDL.Variant({
'Name' : SortByDirection,
});
Expand Down Expand Up @@ -1403,11 +1409,6 @@ export const idlFactory = ({ IDL }) => {
}),
'Err' : Error,
});
const ListRequestsSortBy = IDL.Variant({
'ExpirationDt' : SortByDirection,
'LastModificationDt' : SortByDirection,
'CreatedAt' : SortByDirection,
});
const RequestStatusCode = IDL.Variant({
'Failed' : IDL.Null,
'Approved' : IDL.Null,
Expand Down
1 change: 1 addition & 0 deletions apps/wallet/src/services/station.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -481,6 +481,7 @@ export class StationService {
const result = await actor.get_next_approvable_request({
operation_types: types ? [types] : [],
excluded_request_ids: excludedRequestIds ?? [],
sort_by: [],
});

if (variantIs(result, 'Err')) {
Expand Down
2 changes: 2 additions & 0 deletions core/station/api/spec.did
Original file line number Diff line number Diff line change
Expand Up @@ -1385,6 +1385,8 @@ type GetNextApprovableRequestInput = record {
operation_types : opt vec ListRequestsOperationType;
// Exclude requests the user indicated to skip.
excluded_request_ids : vec UUID;
// Get the next request from a list sorted by the given field.
sort_by : opt ListRequestsSortBy;
};

// Result type for retrieving a request.
Expand Down
1 change: 1 addition & 0 deletions core/station/api/src/request.rs
Original file line number Diff line number Diff line change
Expand Up @@ -313,6 +313,7 @@ pub struct ListRequestsResponse {
pub struct GetNextApprovableRequestInput {
pub excluded_request_ids: Vec<UuidDTO>,
pub operation_types: Option<Vec<ListRequestsOperationTypeDTO>>,
pub sort_by: Option<ListRequestsSortBy>,
}

pub type GetNextApprovableRequestResponse = Option<GetRequestResponse>;
Expand Down
159 changes: 154 additions & 5 deletions core/station/impl/src/services/request.rs
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,15 @@ impl RequestService {
vec![]
};

let mut statuses: Vec<RequestStatusCode> = input
.statuses
.map(|statuses| statuses.into_iter().map(Into::into).collect::<_>())
.unwrap_or_default();

if input.only_approvable && !statuses.contains(&RequestStatusCode::Created) {
statuses.push(RequestStatusCode::Created);
}

let mut request_ids = self.request_repository.find_ids_where(
RequestWhereClause {
created_dt_from: input
Expand All @@ -197,10 +206,7 @@ impl RequestService {
.collect::<_>()
})
.unwrap_or_default(),
statuses: input
.statuses
.map(|statuses| statuses.into_iter().map(Into::into).collect::<_>())
.unwrap_or_default(),
statuses,
requesters: filter_by_requesters.unwrap_or_default(),
approvers: filter_by_approvers.unwrap_or_default(),
not_approvers: filter_by_votable.clone(),
Expand Down Expand Up @@ -288,7 +294,7 @@ impl RequestService {
not_requesters: filter_by_votable,
excluded_ids: exclude_request_ids,
},
None,
input.sort_by,
)?;

// filter out requests that the caller does not have access to read
Expand Down Expand Up @@ -1303,6 +1309,149 @@ mod tests {
assert_eq!(votable_requests.items[0].id, transfer_requests[0].id);
assert_eq!(votable_requests.items[1].id, transfer_requests[2].id);
}

#[tokio::test]
async fn get_next_approvable_request_returns_requests_sorted() {
let ctx = setup();

let user_2 = mock_user();
USER_REPOSITORY.insert(user_2.to_key(), user_2.clone());

REQUEST_POLICY_REPOSITORY.insert(
[0; 16],
RequestPolicy {
id: [0; 16],
specifier: RequestSpecifier::Transfer(ResourceIds::Any),
rule: RequestPolicyRule::QuorumPercentage(
UserSpecifier::Id(vec![ctx.caller_user.id, user_2.id]),
Percentage(100),
),
},
);

let mut request = mock_request();
request.status = RequestStatus::Created;
request.approvals = vec![RequestApproval {
approver_id: ctx.caller_user.id,
status: RequestApprovalStatus::Approved,
decided_dt: 0,
last_modification_timestamp: 0,
status_reason: None,
}];

// Add 3 requests with different ids and created_timestamps.
// The requests in the BTreeMap are stored ordered by their id.
// Without a sort_by, the request with the lowest id should be returned first,
// which will be in reverse order of creation.
for (id, ts) in [(1u8, 3u64), (4, 2), (3, 1), (2, 4)] {
request.id = [id; 16]; // storage order is different from timestamp order
request.created_timestamp = ts;
REQUEST_REPOSITORY.insert(request.to_key(), request.clone());
}

let next_approvable_request = ctx
.service
.get_next_approvable_request(
GetNextApprovableRequestInput {
excluded_request_ids: vec![],
operation_types: None,
sort_by: None,
},
Some(&CallContext::new(user_2.identities[0])),
)
.await
.expect("Failed to get next approvable request")
.expect("No next approvable request");

// without sort_by, the request with the highest timestamp should be returned
assert_eq!(next_approvable_request.created_timestamp, 4);

let next_approvable_request = ctx
.service
.get_next_approvable_request(
GetNextApprovableRequestInput {
excluded_request_ids: vec![],
operation_types: None,
sort_by: Some(station_api::ListRequestsSortBy::CreatedAt(
station_api::SortDirection::Asc,
)),
},
Some(&CallContext::new(user_2.identities[0])),
)
.await
.expect("Failed to get next approvable request")
.expect("No next approvable request");

// with sort_by, the request with the lowest created_timestamp should be returned
assert_eq!(next_approvable_request.created_timestamp, 1);
}

#[tokio::test]
async fn list_approvable_only_lists_pending() {
let ctx = setup();

let user_2 = mock_user();
USER_REPOSITORY.insert(user_2.to_key(), user_2.clone());

REQUEST_POLICY_REPOSITORY.insert(
[0; 16],
RequestPolicy {
id: [0; 16],
specifier: RequestSpecifier::Transfer(ResourceIds::Any),
rule: RequestPolicyRule::QuorumPercentage(
UserSpecifier::Id(vec![ctx.caller_user.id, user_2.id]),
Percentage(100),
),
},
);

let approval = RequestApproval {
approver_id: ctx.caller_user.id,
status: RequestApprovalStatus::Approved,
decided_dt: 0,
last_modification_timestamp: 0,
status_reason: None,
};

let mut non_approvable_request = mock_request();
non_approvable_request.status = RequestStatus::Completed { completed_at: 0 };
non_approvable_request.approvals = vec![approval.clone()];
ctx.repository.insert(
non_approvable_request.to_key(),
non_approvable_request.clone(),
);

let mut approvable_request = mock_request();
approvable_request.status = RequestStatus::Created;
approvable_request.approvals = vec![approval.clone()];
ctx.repository
.insert(approvable_request.to_key(), approvable_request.clone());

let approvable_requests = ctx
.service
.list_requests(
ListRequestsInput {
requester_ids: None,
approver_ids: None,
statuses: None,
operation_types: None,
expiration_from_dt: None,
expiration_to_dt: None,
created_from_dt: None,
created_to_dt: None,
paginate: None,
sort_by: None,
only_approvable: true,
with_evaluation_results: false,
},
&CallContext::new(user_2.identities[0]),
)
.await
.expect("Failed to list approvable requests");

assert_eq!(approvable_requests.items.len(), 1);
assert_eq!(approvable_requests.items[0].id, approvable_request.id);
}
}

#[cfg(feature = "canbench")]
Expand Down
1 change: 1 addition & 0 deletions tests/integration/src/dfx_orbit/review.rs
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@ fn review() {
operation_types: Some(vec![ListRequestsOperationTypeDTO::CallExternalCanister(
Some(canister_id),
)]),
sort_by: None,
})
.await
.unwrap()
Expand Down
1 change: 1 addition & 0 deletions tools/dfx-orbit/src/review.rs
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ impl From<ReviewNextArgs> for GetNextApprovableRequestInput {
Self {
excluded_request_ids: vec![],
operation_types: (!args.any).then(external_canister_operations),
sort_by: None,
}
}
}
Expand Down
Loading