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

Feature: allow replacing project with template #1889

Merged
merged 31 commits into from
Dec 10, 2018

Conversation

magicznyleszek
Copy link
Member

@magicznyleszek magicznyleszek commented Jul 25, 2018

Description

Replacing project with template implementation. FE uses PATCH on asset with clone_from parameter (there is a bug, see related issues).

Related issues

fixes #1887
fixes #1810
blocked by #1892

@magicznyleszek magicznyleszek changed the base branch from master to form-template-frontend July 25, 2018 13:25
@pmusaraj
Copy link
Contributor

Something isn't right here. After replacing a project from a template, I get an error when trying to preview in Enketo.

Also, sometimes if I go to the form builder immediately after replacing with a template, I see the old form questions in the builder. I would need to refresh to see the new questions.

@pmusaraj pmusaraj changed the base branch from form-template-frontend to master July 29, 2018 01:47
@magicznyleszek magicznyleszek changed the title Replace project with template Feature: allow replacing project with template Aug 13, 2018
and rename onComplete to onCompleted for consistency
plus rename onComplete to onCompleted and move update asset error notification to actions
@magicznyleszek
Copy link
Member Author

@pmusaraj ready to be tested :)

Copy link
Contributor

@pmusaraj pmusaraj left a comment

Choose a reason for hiding this comment

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

I tested this branch both locally and on a demo (du), and got a 500 error when trying to replace a project with a template.

@@ -383,17 +383,21 @@ actions.resources.listTags.completed.listen(function(results){
}
});

actions.resources.updateAsset.listen(function(uid, values, params={}) {
actions.resources.updateAsset.listen(function(uid, values, callbacks={}) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't change params to callbacks here, and we shouldn't change onComplete to onCompleted below. It's not necessary to make these changes to nomenclature, it is longer to review and it's also prone to merge conflicts (the searchStore branch makes changes in the same lines, roughly).

jsapp/js/actions.es6 Outdated Show resolved Hide resolved
@magicznyleszek
Copy link
Member Author

@pmusaraj I undid all these unnecessary naming changes and custom callbacks (and will try embrace natural Reflux actions from now on :-P)

@pmusaraj
Copy link
Contributor

Naming changes look good.

@magicznyleszek
Copy link
Member Author

Currently blocked by BE issue: dictionary update sequence element #0 has length 1; 2 is required

@magicznyleszek
Copy link
Member Author

@pmusaraj BE issue fixed, ready to be reviewed and merged :)

@magicznyleszek
Copy link
Member Author

@pmusaraj ping :)

@pmusaraj
Copy link
Contributor

pmusaraj commented Oct 3, 2018

Tested this on unu @magicznyleszek and I still have an issue: after replacing a project with a template, when I go to the form builder, I see the project's original questions. I should see the new questions, from the template.

@magicznyleszek
Copy link
Member Author

magicznyleszek commented Oct 5, 2018

@pmusaraj fond the problem - Form Builder uses allAssets store, which unlike searches wasn't being updated on updateAsset action. This can also fix some other unknown bugs. Deployed on unu for retest :)

@magicznyleszek
Copy link
Member Author

@pmusaraj ping ;-)

@jnm
Copy link
Member

jnm commented Dec 6, 2018

I tested this and it seems to work well. Penar may not have time to do any more reviews. I'd be happy to merge once the order is changed as I indicated above.

@jnm jnm merged commit 63968cc into master Dec 10, 2018
@jnm jnm deleted the replace-project-with-template branch December 10, 2018 19:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants