-
Notifications
You must be signed in to change notification settings - Fork 8
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
Adding topic detail model #39
base: master
Are you sure you want to change the base?
Conversation
@amalagaura more just submitting this for your comment - is there a better way we could do this? |
77b04ce
to
71c852e
Compare
@dannykopping This seems ok, let me take a look at this and see what else I can find. Give me a day or two. |
@dannykopping Any thoughts on using Her's nested attributes? I think it would mean creating a Her model for TopicDetail, which would be unusable. Then we would add a |
OK I see you don't care about the deserialization as much. It is only used for |
Correct, ya. It's purely a request concern |
@dannykopping Yes I think I added the fetching and locking as an after thought. It was just a bare minimum. I didn't have any complex requirements for that. I think your PR will work. You can add tests and I can merge unless you want to add anything else. |
Sweet, thanks Ankur - we'll add the tests tomorrow hopefully |
…ents in the fetchAndLock call
1d0aa5c
to
42c0191
Compare
Adding topic detail model to express additional topic detail requirements in the fetchAndLock call