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 #1007 -- small improvements to training request workflow #1030

Merged
merged 8 commits into from Oct 8, 2016

Conversation

chrismedrela
Copy link
Contributor

No description provided.

Until now, pending requests can't be matched with a person and accepted
requests must be matched with a person.

Now, pending requests can't be matched with a *training* and accepted
requests may or may not be matched with a training. It doesn't matter
whether a request is matched with a person.
because it's unaligned with other buttons due to using custom layout.
@chrismedrela chrismedrela changed the title Fix 1007 Fix #1007 -- small improvements to training request workflow Sep 28, 2016
@gvwilson
Copy link
Contributor

gvwilson commented Oct 6, 2016

Looks good to me - please merge when you're ready.

@gvwilson gvwilson assigned chrismedrela and unassigned gvwilson Oct 6, 2016
('p', 'Pending'), # initial state
('a', 'Accepted'), # state after matching a Person record
('d', 'Discarded'),
)
]
Copy link
Contributor

Choose a reason for hiding this comment

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

why a list rather than a tuple?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because here it's more readable to write:

[('', 'Pending or accepted')] + TrainingRequest.STATES

rather than

(('', 'Pending or accepted'),) + TrainingRequest.STATES

if self.state == 'p' and self.person is not None \
and self.person.get_training_tasks().exists():
raise ValidationError({'state': 'Pending training request cannot '
'be matched with a training.'})
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch 👍

('p', 'Pending'), # initial state
('a', 'Accepted'), # state after matching a Person record
('d', 'Discarded'),
)
]
Copy link
Contributor

Choose a reason for hiding this comment

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

Why a list rather than a tuple?

raise ValidationError({'person': 'Pending training requests cannot '
'be matched to a person.'})
if self.state == 'p' and self.person is not None \
and self.person.get_training_tasks().exists():
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch - thanks.

@gvwilson
Copy link
Contributor

gvwilson commented Oct 6, 2016

Looks good to me - please merge when you're ready.

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.

None yet

3 participants