PR isolating model changes for validation

[tiagoandresvaz]

I have a clone of my forked repo on Github and started developing a few features for our local region (RS - State of Rio Grande do Sul) in Brazil and also changes in the model to introduce a new flux as described in #703.

The problem is, I did that in my master branch which now contains scenarios and the new changes in the model :-(

That broke my ability to create a reasonable pull request isolating model changes, as pointed by @rneher Richard.

So I want to do the following procedure:

1. Restore my fork master branch to be the same as the upstream master.
2. Create a new branch (cenarios_rs). and commit local scenarios changes to the new branch.
3. Create a new branch (palliative_flux) and commit model changes to the new branch.
4. Create a PR from palliative_flux with model changes to upstream
5. then, merge cenarios_rs and palliative_flux to my local master branch, so I have a full version for my local team to do the work.

Commands to do that:

$ git branch cenarios_rs
$ git branch palliative_flux

-- reset to the previous commit before my first contribs and update from upstream

$ git checkout master
$ git reset --hard f623d64c66a5d5bbc7f4503199fb3f9335b219bb
$ git pull upstream master

--Then I DELETE my master on remote repo on GitHub 
--going to the admin section in my fork and setting the default branch to palliative_flux. To recreate it.

$ git push origin :master
$ git push origin master

--Set back my fork default to master on GitHub.

in the future, I will do all my work in branches and create pull requests from them.

Leaving my master branch alone and just merging down whatever comes in from upstream.

Is this a reasonable approach to follow in further contributions to the project?

[rneher]

yes, this seems more or less reasonable. But create the new branches on master from upstream.

git checkout neherlab/master
git branch cenarios_rs
git branch palliative_flux

Then pick the changes you made in your master onto these new branches. you can either do this with the command line, or with a GUI tool like gitkraken.

I don't think you have to set your default branch to palliative_flux, you can create a PR from every branch to every branch. I'll have a look at your changes to the model in the meantime. .

[rneher]

Just a quick comment on your PR. Overall, the modifications to the model look sensible. One thing, however, that needs validating is that palliative and critical together don't exceed 100%. The form validation needs to catch this. The other issue is that we need to make sure the import of scenarios and the url serialization continue to work.

[ivan-aksamentov]

@tiagoandresvaz Hi Tiago,

Both me and Richard, we reviewed the PR at the end of last week and I was worried that you closed the PR because we were not responsive for some time. Sorry for the delay, we had some work on another project.

Sure, the workflow you described is reasonable. Let's do that. One thing it will solve is that we don't need to also merge your UI style changes and the change of the default scenario, for example. You can have these customizations on your fork separately for Brazil, and we can stick with the "international" ones.

Feel free to resubmit the PR.

Let us know if you have other suggestions or ideas, regarding local specifics in Brazil or otherwise. We always glad to hear from people in different parts of the world.

[tiagoandresvaz]

Hi Ivan, It was for a good reason. Now I have things more organized here on my side. I am going to create a new branch for palliative flux on upstream and create a new PR from there! Thanks for your reply 👍 Em dom, 12 de jul de 2020 às 23:30, Ivan Aksamentov < [email protected]> escreveu:

@tiagoandresvaz <https://github.com/tiagoandresvaz> Hi Tiago, Both me and Richard, we reviewed the PR at the end of last week and I was worried that you closed the PR because we were not responsive for some time. Sorry for the delay, we had some work on another project. Sure, the workflow you described is reasonable. Let's do that. One thing it will solve is that we don't need to also merge your UI style changes and the change of the default scenario, for example. You can have these customizations on your fork separately for Brazil, and we can stick with the "international" ones. Feel free to resubmit the PR. Let us know if you have other suggestions or ideas, regarding local specifics in Brazil or otherwise. We always glad to hear from people in different parts of the world. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#753 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFXWDNTF6ZOTRNMXZ2FBWLR3JWSXANCNFSM4OXMGOPA> .

-- Obrigado, Tiago Vaz

[tiagoandresvaz]

PR done #757

validation for palliative +critical <=100% is done, but eslint is not happy here:

severity: yup
.array()
.of(
yup
.object()
.shape({
id: yup.string().required(MSG_REQUIRED),
ageGroup: yup.string().oneOf(Object.values(AgeGroup)).required(MSG_REQUIRED),
population: positiveIntegerSchema.required(MSG_REQUIRED),
confirmed: percentageSchema.required(MSG_REQUIRED),
severe: percentageSchema.required(MSG_REQUIRED),
palliative: percentageSchema.required(MSG_REQUIRED),
critical: percentageSchema.required(MSG_REQUIRED).test('max',MSG_EXCEED_100,function valExceed(value)
{
const { palliative } = this.parent
return value <= 100 - palliative
}),
fatal: percentageSchema.required(MSG_REQUIRED),
isolated: percentageSchema.required(MSG_REQUIRED),
})
.required(MSG_REQUIRED),
)

also, when users load an previous version of .json scenario, and palliative info is not found it is not breaking the app, but also is not filling zeros automatically - any suggestion on this?

[ivan-aksamentov]

@tiagoandresvaz I answered in the PR conversation #757