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

Bug claims pending #80

Merged
merged 4 commits into from
Oct 15, 2019
Merged

Bug claims pending #80

merged 4 commits into from
Oct 15, 2019

Conversation

kjgarza
Copy link
Contributor

@kjgarza kjgarza commented Oct 10, 2019

write_attribute does not save to active record.


This change is Reviewable

write_attribute does not save to active record.
include dependcy injection for better testing
@kjgarza kjgarza requested a review from mfenner October 10, 2019 17:39
@kjgarza kjgarza self-assigned this Oct 10, 2019
Copy link
Contributor

@mfenner mfenner left a comment

Choose a reason for hiding this comment

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

Thanks for catching the obsolete env var. Do your changes fix the state machine issue?

@mfenner
Copy link
Contributor

mfenner commented Oct 11, 2019

@kjgarza The saving to ActiveRecord should happen via the aasm_state gem: https://github.com/aasm/aasm#activerecord

@kjgarza
Copy link
Contributor Author

kjgarza commented Oct 11, 2019

ok, I can see that. In the validation I did, that does not seem to be working (saving to AR) . Probably , the transitions calls are missing the Bang!.

job.aasm.fire!(:run) # saved

I'll check the AASM docs

@mfenner
Copy link
Contributor

mfenner commented Oct 11, 2019

Thanks! If aasm_state saves to active_record, write_attribute makes sense, reducing the number of database calls. Also, this was the working code used before switching to aasm_state, see commit history.

You can tell AASM to auto-save the object or leave it unsaved
@kjgarza kjgarza merged commit f3c263c into master Oct 15, 2019
@mfenner mfenner deleted the bug_claims_pending branch October 20, 2019 22:00
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.

2 participants