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

v8 - Remove legacy drop downs #3414

Merged
merged 11 commits into from Oct 30, 2018

Conversation

@Shazwazza
Copy link
Member

commented Oct 24, 2018

Prerequisites

  • I have created an issue for the proposed changes in this PR, the link is: This is the issue :)
  • I have added steps to test this contribution in the description below

Description

This removes all of the legacy drop down property editors and deprecated logic. We now only have one Drop down property editor and it only stores values in the database and cache in a single way which is the string values that have been selected. There is no more 'publishing keys' type of stuff and we no longer store the INT ids that were based on the pre-value items (which also no longer makes sense because pre-values are just json configuration in v8). There are various other fixes too like the 'multiple' option for drop downs was broken.

Testing

  • Before testing this, be sure that in the normal temp8 branch, you have data types made out of the old property editors. To do this you will need to enable the umbraco config setting showDeprecatedPropertyEditors. Make a data type of: DropDownListMultiple, DropdownlistMultiplePublishKeys, DropDownList, DropdownlistPublishKeys and the DropDownListFlexible (one for single and one for multiple) and fill in pre-values for all of them and create a few content items that use all of these data types with data stored for them
  • Have a template for these content items that show the values of these properties - the ones that are 'publishing keys' should display INT values (this is legacy)
  • Then checkout this branch/PR, it will run the upgrader which will convert all of your data. It will change all of these data types to use the DropDownListFlexible with the correct 'multiple' parameter applied. It will also convert all of the data stored in the database from storing INT Ids to STRING values
  • Test that the front-end values are displayed correctly - there should be no more INT values displayed, they will all be the STRING values
  • Test that the back office content editors all still work as expected
  • Test that the back office data type editors all still work as expected
  • Test that the multiple option for drop downs now works (i.e. you can select multiple items)
…own editor now and it doesn't store IDs either in the db or in the cache, it just stores what is selected
@ghost ghost assigned Shazwazza Oct 24, 2018
@Shazwazza Shazwazza changed the base branch from dev-v7 to temp8 Oct 24, 2018
@Shazwazza Shazwazza added this to the sprint96 milestone Oct 24, 2018
@Shazwazza Shazwazza removed their assignment Oct 24, 2018
…, fixes flexible drop down angular editor to post string vals, not the int ids, removes old angular drop down prop editor
@ghost ghost assigned Shazwazza Oct 24, 2018
Shazwazza added 2 commits Oct 24, 2018
@warrenbuckley

This comment has been minimized.

Copy link
Member

commented Oct 25, 2018

@Shazwazza I had the following issue/s after the migration had run

All properties were converted to use the Flexible dropdown, however, the DropdownListMultiple did not have the toggle for the multiple enabled correctly, where the other multiple variants were correctly set.

Next issue is that my page was no longer in the cache, so I republished the node and got the following exception (see screenshot) where it was not expecting a string but an integer instead.

image

I then thought OK I would remove these properties from my doctype so I could sucesfully publish the node without an error being thrown, however the node was still reporting that it was published but not in the cache.

After going to the settings section and using the Published Status dashbaord aka NuCache. Clicking the Reload button throws the following exception - see YSOD below. So I currently can not see my page & values rendering out from the template to see if some of this was sucessful or not.

image

}
catch (Exception ex)
{
Logger.Error<DropDownPropertyEditorsMigration>(ex, $"Invalid drop down configuration detected: \"{dd.Configuration}\", cannot convert editor, values will be cleared");

This comment has been minimized.

Copy link
@warrenbuckley

warrenbuckley Oct 25, 2018

Member

Try not to do string.interpolation for Serilog log messages

Suggested change
Logger.Error<DropDownPropertyEditorsMigration>(ex, $"Invalid drop down configuration detected: \"{dd.Configuration}\", cannot convert editor, values will be cleared");
Logger.Error<DropDownPropertyEditorsMigration>(ex, "Invalid drop down configuration detected: \"{DropDownConfiguration}\", cannot convert editor, values will be cleared", dd.Configuration);
vals.Add(val.Value);
else
{
Logger.Warn<DropDownPropertyEditorsMigration>($"Could not find associated data type configuration for stored Id {id}");

This comment has been minimized.

Copy link
@warrenbuckley

warrenbuckley Oct 25, 2018

Member

Please don't do string interpolation of logging messages - otherwise, the Serilog template is not unique

Suggested change
Logger.Warn<DropDownPropertyEditorsMigration>($"Could not find associated data type configuration for stored Id {id}");
Logger.Warn<DropDownPropertyEditorsMigration>("Could not find associated data type configuration for stored Id {Id}", id);
@madsrasmussen

This comment has been minimized.

Copy link
Contributor

commented Oct 25, 2018

I get similar test results as Warren.

I created a doctype with one of each dropdown type. After upgraded they were correctly updated to the new flexible dropdown type. My data on the node didn't get updated correctly and I lost the selection of most of my property editors.

Screenshot 2018-10-25 13.42.52.png

The frontend still shows integers after upgrade:
Screenshot 2018-10-25 13.46.34.png

If I try to republish the node I get a YSOD:
"Cannot assign value "Tree" of type "System.String" to property "dropdownListMultiplePublishingKeys" expecting type "System.Int32"."

@Shazwazza

This comment has been minimized.

Copy link
Member Author

commented Oct 26, 2018

Cool, thanks for testing, i'll give it a try. There's a TODO in the code about regenerating the cache, i need @zpqrtbnk to tell me how to do that :)

@clausjensen clausjensen modified the milestones: sprint96, sprint97 Oct 29, 2018
@Shazwazza Shazwazza self-assigned this Oct 29, 2018
Shazwazza added 3 commits Oct 29, 2018
…, fixes boolean logic on the js single drop down
Shazwazza added 2 commits Oct 29, 2018
…drop-downs

# Conflicts:
#	src/Umbraco.Core/Migrations/Upgrade/UmbracoPlan.cs
@Shazwazza Shazwazza removed their assignment Oct 30, 2018
@Shazwazza

This comment has been minimized.

Copy link
Member Author

commented Oct 30, 2018

I've pushed fixes for the above, only one thing left to do and fix the front-end cache refreshing

@warrenbuckley warrenbuckley self-assigned this Oct 30, 2018
@warrenbuckley

This comment has been minimized.

Copy link
Member

commented Oct 30, 2018

@Shazwazza all re-tested and works all fine now, merged in 🎉

@warrenbuckley warrenbuckley merged commit 3e3f2a9 into temp8 Oct 30, 2018
1 check failed
1 check failed
Cms 8 Continuous #0934 failed
Details
@ghost ghost removed the state/review label Oct 30, 2018
@Shazwazza

This comment has been minimized.

Copy link
Member Author

commented Oct 31, 2018

@warrenbuckley great! make sure you delete branches after you've merged :) (i'll get this one deleted)

I also still need to do what i mentioned above and ensure the cache is regenerated in some cases, i'll get to that this week though.

@Shazwazza Shazwazza deleted the temp8-remove-legacy-drop-downs branch Oct 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.