Skip to content

Adding topic detail model#39

Draft
dannykopping wants to merge 3 commits into
amalagaura:masterfrom
xneelo:dynamic-topic-properties
Draft

Adding topic detail model#39
dannykopping wants to merge 3 commits into
amalagaura:masterfrom
xneelo:dynamic-topic-properties

Conversation

@dannykopping

Copy link
Copy Markdown
Contributor

Adding topic detail model to express additional topic detail requirements in the fetchAndLock call

@dannykopping

Copy link
Copy Markdown
Contributor Author

@amalagaura more just submitting this for your comment - is there a better way we could do this?
In my use-case, we need to be able to send the processDefinitionVersionTag param as part of the fetchAndLock call

@dannykopping dannykopping force-pushed the dynamic-topic-properties branch from 77b04ce to 71c852e Compare January 22, 2020 09:38
@amalagaura

Copy link
Copy Markdown
Owner

@dannykopping This seems ok, let me take a look at this and see what else I can find. Give me a day or two.

@amalagaura

Copy link
Copy Markdown
Owner

@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 has_many and accepts_nested_attributes_for and then alias topics topics_attributes. But you get the automatic Ruby class for TopicDetail.

@amalagaura

Copy link
Copy Markdown
Owner

OK I see you don't care about the deserialization as much. It is only used for fetch_and_lock. This make sense then. It is just a PORO instead of passing in a hash I see.

@dannykopping

Copy link
Copy Markdown
Contributor Author

OK I see you don't care about the deserialization as much. It is only used for fetch_and_lock. This make sense then. It is just a PORO instead of passing in a hash I see.

Correct, ya. It's purely a request concern
As we use the API more and build out more support, it might be a nice idea to create a "request model" concept in the lib which all of these can be based off of

@amalagaura

Copy link
Copy Markdown
Owner

@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.

@dannykopping

Copy link
Copy Markdown
Contributor Author

Sweet, thanks Ankur - we'll add the tests tomorrow hopefully

@dannykopping dannykopping force-pushed the dynamic-topic-properties branch from 1d0aa5c to 42c0191 Compare February 25, 2020 06:42
@amalagaura amalagaura marked this pull request as draft September 4, 2020 01:24
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