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

TerminateAgreement endpoint #832

Merged
merged 18 commits into from
Dec 8, 2020
Merged

TerminateAgreement endpoint #832

merged 18 commits into from
Dec 8, 2020

Conversation

jiivan
Copy link
Contributor

@jiivan jiivan commented Nov 26, 2020

fixes: #727

(just a branch name change from #792 to trigger market tests)

@jiivan jiivan requested a review from a team November 26, 2020 11:35
Copy link
Contributor

@nieznanysprawiciel nieznanysprawiciel left a comment

Choose a reason for hiding this comment

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

I realized that we can't distinguish between Provider and Requestor Agreement in REST endpoints (like in GetAgreement), but here the problem is much bigger, because we have to know it, to fill terminator field in AgreementTerminatedEvent. But the way note, that we broke GetAgreement endpoint by adding AppSessionId.
I'm afraid we have to address these issues on specification level. @tworec

core/market/src/db/dao/agreement.rs Show resolved Hide resolved
reason: Option<String>,
owner_type: OwnerType,
) -> DbResult<bool> {
log::debug!("Termination reason: {:?}", reason);
Copy link
Contributor

Choose a reason for hiding this comment

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

This will be ugly log:
Some("Reason")
Maybe you should implement something like .display()

core/market/src/negotiation/common.rs Outdated Show resolved Hide resolved
core/market/src/negotiation/common.rs Outdated Show resolved Hide resolved
core/market/src/negotiation/common.rs Outdated Show resolved Hide resolved
core/market/src/rest_api/provider.rs Outdated Show resolved Hide resolved
core/market/src/negotiation/common.rs Outdated Show resolved Hide resolved
core/market/src/negotiation/common.rs Show resolved Hide resolved
core/market/src/negotiation/common.rs Show resolved Hide resolved
Comment on lines 290 to 295
let event = NewAgreementEvent {
agreement_id: id.clone(),
reason: reason,
event_type: AgreementEventType::Terminated,
issuer: owner_type,
};
Copy link
Contributor

Choose a reason for hiding this comment

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

signature is not optional for AgreementTerminatedEvent but this is part of #781

Comment on lines 601 to 605
if let Some(s) = reason {
Reason::from_json_reason(JsonReason {
json: serde_json::from_str(s)?,
})?;
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if let Some(s) = reason {
Reason::from_json_reason(JsonReason {
json: serde_json::from_str(s)?,
})?;
};
if let Some(s) = reason {
serde_json::from_str::<Reason>(s)?
};

Copy link
Contributor

Choose a reason for hiding this comment

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

Or even:

    Ok(if let Some(s) = reason {
        serde_json::from_str::<Reason>(s)?;
    })

@@ -431,7 +431,14 @@ impl CommonBroker {

dao.terminate(&agreement_id, msg.reason, owner_type)
.await
.map_err(|_e| RemoteAgreementError::InternalError(agreement_id.clone()))?;
.map_err(|e| {
log::info!(
Copy link
Contributor

Choose a reason for hiding this comment

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

I would say it is even warn, because it would leave Provider and Requestor in inconsistent state

verify_reason(reason.as_ref())?;
let dao = self.db.as_dao::<AgreementDao>();
log::debug!(
"Getting agreement. id: {:?}, agrid: {}, reason: {:?}",
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't display using debug if it is possible

@nieznanysprawiciel nieznanysprawiciel merged commit 32b8050 into master Dec 8, 2020
@nieznanysprawiciel nieznanysprawiciel deleted the market/termag-ep branch December 8, 2020 08:56
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.

Terminate Agreement endpoint
2 participants