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

Test to confirm error in Archetype Collection creation #410

Closed
wants to merge 1 commit into from
Closed

Test to confirm error in Archetype Collection creation #410

wants to merge 1 commit into from

Conversation

idgserpro
Copy link

@idgserpro idgserpro commented Aug 14, 2018

refs. #413

@idgserpro
Copy link
Author

@hvelarde note that the error that occurs in creating the collection occurs here as well.

@hvelarde
Copy link
Member

now we must open an issue describing the problem :)

@hvelarde
Copy link
Member

hvelarde commented Sep 4, 2018

@gbastien already opened the issue for us.

@gbastien
Copy link
Member

gbastien commented Sep 4, 2018

Hi @idgserpro I did not see this PR... We have the same problem, still need to decide how to fix it...

Gauthier

@gbastien
Copy link
Member

gbastien commented Sep 5, 2018

Hi @idgserpro @hvelarde

I can confirm that if I change {None: None} to {'':''}, bool({'':''} is still True so it is used by AT's processForm and the test you added pass as well as the test_create_at_with_title_in_request that was added regarding the {None: None}.

So I would go for that, what do you think?

Gauthier

@hvelarde
Copy link
Member

hvelarde commented Sep 5, 2018

if tests pass I think it could be fine; just be careful to not reintroduce another issue with Archetypes-based Event content type. I would suggest to add tests on #407 to this pull request as well.

@idgserpro
Copy link
Author

@gbastien I also think {'': ''} will work. I agree with this solution. If you could create another PR would be great. I have no contributor agreement. Feel free to add the test of this PR and the test suggested by @hvelarde.

@gbastien
Copy link
Member

gbastien commented Sep 5, 2018

@idgserpro ok I will create a separated PR and keep your test.

@hvelarde so I would add the tests of PR #407 as well to the new PR to make sure it works with AT and DX event? I totally agree, why PR #407 was not merged?

I will propose a PR tomorrow with all this together.

Thank you for review,

Gauthier

@hvelarde
Copy link
Member

hvelarde commented Sep 5, 2018

@gbastien it was not merged because I though it was unnecessary, but @idgserpro was right and now I think is important to avoid regressions.

@idgserpro
Copy link
Author

Replaced for #414

@idgserpro idgserpro closed this Sep 6, 2018
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.

4 participants