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

Restart negotiations #1150

Merged
merged 20 commits into from
May 19, 2021
Merged

Restart negotiations #1150

merged 20 commits into from
May 19, 2021

Conversation

nieznanysprawiciel
Copy link
Contributor

@nieznanysprawiciel nieznanysprawiciel commented Mar 15, 2021

resolves: #1128

@nieznanysprawiciel nieznanysprawiciel marked this pull request as ready for review March 18, 2021 14:26
@nieznanysprawiciel nieznanysprawiciel requested review from a team, tworec and jiivan March 18, 2021 14:26
@tworec tworec removed their request for review April 26, 2021 08:57
jiivan
jiivan previously approved these changes Apr 26, 2021
Copy link
Contributor

@jiivan jiivan left a comment

Choose a reason for hiding this comment

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

LGTM

@tworec
Copy link
Contributor

tworec commented Apr 26, 2021

I'm still lacking Why section for this PR (linked issue does not have it).

@tworec tworec force-pushed the market/restart-negotiations branch from 3df3c1c to bf186c5 Compare April 27, 2021 00:59
@@ -72,6 +72,10 @@ impl<'c> ProposalDao<'c> {
return Err(SaveProposalError::AlreadyCountered(prev_proposal));
}

// Proposal could be in Rejected state. It is ok to counter Rejected Proposal,
// but we must change state of prev Proposal to Draft.
update_proposal_state(conn, &prev_proposal, ProposalState::Draft)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

this unconditional state change worries me

.unwrap()
.body
.state,
ProposalState::Draft
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd expect some finalized agreement as a conclusion of restarted negotation

Copy link
Contributor

@tworec tworec left a comment

Choose a reason for hiding this comment

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

great job!

@@ -63,18 +63,30 @@ impl<'c> ProposalDao<'c> {
pub async fn save_proposal(&self, proposal: &Proposal) -> Result<(), SaveProposalError> {
let proposal = proposal.body.clone();
do_with_transaction(self.pool, move |conn| {
let prev_proposal = proposal
let prev_proposal_id = proposal
Copy link
Contributor

Choose a reason for hiding this comment

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

👍


// If previous Proposal was rejected, we must change it's state back.
if prev_proposal.state == ProposalState::Rejected {
// If rejected Proposal doesn't have prev_proposal_id, it was Initial Proposal.
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 great comments

@@ -984,7 +985,7 @@ async fn test_restart_negotiations() {
.await
.unwrap();

let prov_proposal_id = req_market
let req_proposal_id = req_market
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
let req_proposal_id = req_market
// now the magic begins. We are countering a rejected proposal
let req_proposal_id = req_market

@@ -1026,6 +1030,45 @@ async fn test_restart_negotiations() {
.state,
ProposalState::Draft
);

// Agents should be able to sign Agreement after rejection.
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

.unwrap()
.body
.state,
ProposalState::Initial
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@nieznanysprawiciel nieznanysprawiciel merged commit 5f3b883 into master May 19, 2021
@nieznanysprawiciel nieznanysprawiciel deleted the market/restart-negotiations branch May 19, 2021 09:43
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.

Allow counter_proposal after Proposal was rejected
3 participants