Skip to content

Product templates use forms #290

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

Merged
merged 2 commits into from
Apr 25, 2019
Merged

Conversation

maxceem
Copy link
Contributor

@maxceem maxceem commented Apr 12, 2019

winning submission from challenge 30088320 - Topcoder Project Service - Product templates use forms

Submission looks good to me.

I think to finish with this refactoring process we can do a couple of more things:

  1. In this PR helper method has been created util.checkModel while in previous code this method has been defined several times:

    So we can reuse a new helper method in all these cases.

  2. At the moment we have two ways of defining forms for both ProductTemplates and ProjectTemplates: old way inside the template and new way using Form model.
    So both of fields are optional:

    image

    and

    image

    For ProductTemplate we should check in create/update endpoints that

    • either form or template field is defined, but not both together.

    For ProjectTemplate we should check in create/update endpoints that:

    • either scope or form field must be defined, but not both together
    • scope or priceConfig cannot be defined together (they can be both null though)
    • either phases or phaseConfig field must be defined, but not both together.

@maxceem maxceem requested a review from vikasrohit April 12, 2019 08:45
@vikasrohit
Copy link

-form or priceConfig cannot be defined together (they can be both null though)

@maxceem I didn't get this statement. IMO, form and priceConfig would be set together in fact.

@vikasrohit vikasrohit merged commit 29ed9d4 into dev Apr 25, 2019
@maxceem
Copy link
Contributor Author

maxceem commented Apr 25, 2019

@vikasrohit right, it was a typo, it should be:

  • scope or priceConfig cannot be defined together (they can be both null though)

I've updated in the comment above.

The main idea that we should validate that we define form template either new or old way, but not both together.

@vikasrohit
Copy link

Okay, got it. Please feel free to run task for the pending items as you listed above, they look good to me.

@vikasrohit vikasrohit deleted the feature/product-templates-refactoring branch July 29, 2019 06:26
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