fecgov / fec-cms

The content management system (CMS) for the new Federal Election Commission website.
https://www.fec.gov
Other
96 stars 39 forks source link

Break out issues that are left that are blocking the wagtail and Django upgrades #2006

Closed PaulClark2 closed 6 years ago

PaulClark2 commented 6 years ago

THIS IS AN INFORMATIONAL ISSUE THAT CONSOLIDATES OUR KNOWN ISSUES

What we're after:

Initial testing of Wagtail 2.0 has revealed some issues that are dealbreakers that must be fixed:

Issues relating to [something about Python] and things that need fixing in the rich text editor (but that aren't dealbreakers) will be in separate tickets. The RTE editor issue is #2024 .

Background:

Initial testing of Wagtail 2.0 page templates and notes: #1973 Write-up of dealbreakers and issues in Rich Text Editor found during initial testing of all templates: https://docs.google.com/document/d/1fgxx9oTEXunm7scqyqJRDF5bG-z45eXO1VkOLwSa8Zw/edit?usp=sharing Related Django and Wagtail updating issues: #1538 and #1968 and #2011 Wagtail 2.0 and hard coded pages #1994 HTML editor issue #1971 and #2024 Python tests failing #1972 The content team's Wagtail wish list: https://docs.google.com/document/d/1HM1-SaxAL-pVhzwQY8ZP-1uA8bVlbAxwFhqW4f-3lAo/edit

Completion criteria:

Annalee has capacity to work on Django issues, tagging @annalee Difficulty - 10 points

llienfec commented 6 years ago

We need to divide up this ticket into several. @pkfec is willing to help!

dorothyyeager commented 6 years ago

I've updated the start of it with the background info I had and references to the other background tickets.

dorothyyeager commented 6 years ago

New ticket written up for the RTE issue and I've edited this issue with the dealbreakers. Now we just need a ticket written up regarding the python issue that affects this.

llienfec commented 6 years ago

One thing that @dorothyyeager and I really need is ability to use anchor links - whether in the editor OR in HTML. It looks like Wagtail is currently enabling this functionality and the work is tracked here: https://github.com/wagtail/wagtail/issues/1049

Not sure if something in their discussion is helpful or if you need us to reach out to their team about a time frame. Happy to do it.

AmyKort commented 6 years ago

Let's plan to get everyone together during this sprint to figure out what Wagtail issued get tackled when and by whom.

dorothyyeager commented 6 years ago

Adding one more item to the mix. Not a dealbreaker, but something @johnnyporkchops suggested I test - scheduled publishing of a page - failed. The page did not go live when I had set it to go live, and I had to manually publish it. Am going to test expiry of the same page tonight (to see if it comes down when I scheduled it to come down).

johnnyporkchops commented 6 years ago

@dorothyyeager , this feature works (at least for dates but not times) on the current version of wagtail we are using. Flagging that we might want to test this again once when we get further along with 2.0 as it might just be a settings issue.

dorothyyeager commented 6 years ago

And just to note that the page did not expire at the time it was set to expire. Will test again as suggested above when we are further along.

fec-jli commented 6 years ago

for Python test, IMO: 1)Can we make sure Django.test TestCase(built-in Django) work first after upgrade Django to 1.11.13? 2) make pytest work (use pytest-django package ??) 3) think about upgrade Django to 2.0.1 4) let Django.test and pytest both work in fec-cms. any suggestion?@annalee Thanks

annalee commented 6 years ago

@fec-jli As I understand our priorities at the moment (correct me if I'm wrong!), we want to:

We're trying to tackle the underlying migrations issue head-on rather than move to PyTest to work around it, because:

So our current plan of action is:

  1. Fix the migrations issue. This will fix Django's test framework.
  2. Write a few catch-all tests in Django's test framework that will tell us if we manage to massively break things.
  3. Upgrade Wagtail and Django, using our new tests to make sure we haven't massively broken anything.
  4. Move to PyTest (starting with moving our existing tests into pytest).

As far as:

4) let Django.test and pytest both work in fec-cms.

@fec-jli can you talk a little more about why we want them both running for fec-cms? The two frameworks can exist side-by-side without causing any problems, but it will be easier to maintain and run our tests if we keep all our tests in one framework or the other.

I personally agree with @patphongs that it makes sense to move to PyTest, since the API is already using it, and because it has more robust features. However, we have a great opportunity here to set up our testing processes from the ground up, so if there are things you prefer about Django's built-in framework, we should talk about it and make sure we're setting up a test process that incorporates everyone's feedback.

patphongs commented 6 years ago

@annalee @fec-jli @rjayasekera @ccostino Thank you for working on this issue. There are 2 separate issues right now in 6.1. I am not sure if it all can be accomplished, please let us know if we should revisit this.

  1. 2011 Fix Migrations and Switch to PyTest from Django

  2. 2006 This current issue to fix dealbreaker wagtail 2.0 issues.

I have made these two issues dependencies before we can merge the upgraded wagtail and django PR https://github.com/fecgov/fec-cms/pull/1968

annalee commented 6 years ago

@patphongs What do you think about splitting #2011 into two issues, since switching to PyTest is not a blocker on upgrading Wagtail, if we're fixing migrations separately?

fec-jli commented 6 years ago

@annalee Thank You for your explanation, I just join this ticket recently. may not know more detail for the progress. I agree with your plan: 1)Fix the migrations issue. This will fix Django's test framework. 2)Write a few catch-all tests in Django's test framework that will tell us if we manage to massively break things. 3)Upgrade Wagtail and Django, using our new tests to make sure we haven't massively broken anything. 4)Move to PyTest (starting with moving our existing tests into pytest). question for 1), after fix migrations issue, will fix Django's test, that is just what I want:) question for 3), which version will we upgrade finally? Django 2.0? Wagtail 2.0.1 ? right?

after we done with 4), both Django's test feature and py.test can be used in fec-cms, right? I am not mean we will use both. sorry about confusion. Thank you so much.

annalee commented 6 years ago

Ah, yes, @fec-jli--sorry for the confusion. Both frameworks will be able to run against the tests when we're done.

patphongs commented 6 years ago

@annalee I agree! Let's split out switching to PyTest and fixing the migrations.

annalee commented 6 years ago

Those are now split into #2040 and #2041, with #2040 in 6.1 and #2041 sitting in "new issues" to be groomed into a later sprint.

dorothyyeager commented 6 years ago

Dealbreakers/Must fix before initial use:

Details are in the content team google doc at https://docs.google.com/document/d/1fgxx9oTEXunm7scqyqJRDF5bG-z45eXO1VkOLwSa8Zw/edit?usp=sharing).

apburnes commented 6 years ago

Reviewed latest editor updates and deal breakers for wagtail upgrade with content team.

Final ToDo

apburnes commented 6 years ago

Closing because remaining deal breakers are addressed in #1968