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

Moved ImportExport cookie consent settings to correct project #2431

Merged
merged 1 commit into from
Oct 29, 2018

Conversation

valadas
Copy link
Contributor

@valadas valadas commented Oct 26, 2018

Closes #2430

Summary

Moved ImportExport cookie consent settings sql script into correct project.

In #2369 an sql script was added to modify the import/export settings in the sql table, but that script being in the Dnn installation and not on the module scripts caused a clean Dnn install to work due to the fact that all Dnn install scripts run first and then the modules install. So the old way would have only worked on upgrades because that column would already exist.

On new installations, it was trying to update a column that did not already exist.

This pull request brings that script at the correct place an allows for a clean install of Dnn to work.

@sleupold
Copy link
Contributor

sleupold commented Oct 26, 2018

@valadas would you mind to check in your install, whether importexportsettings table has been created using proper oq, thank you (I doubt this was caused by using the script).

@valadas
Copy link
Contributor Author

valadas commented Oct 26, 2018

@sleupold when I initially opened the issue, I tought it was because of the object qualifier, but after investigating, it was also the case without an OQ.

So I digged deeper and it is because the original script (that was part of the site scripts) was trying to modify a column on a table that does not exist yet. That table is created by the ExportImport module and module installation runs after the site scripts.

So I moved that into the module sql scripts so it runs after the module is installed.

@sleupold
Copy link
Contributor

@valadas
In this case, the best solution would be moving the SQL Script from Export/Import module to core scripts to avoid module dependencies.

@valadas
Copy link
Contributor Author

valadas commented Oct 26, 2018

Maybe... I suggest merging this for now because it solves the Dnn install issue and it follows how it is implemented now (as a module) and you could open a new issue to move the whole module into the API.

Copy link
Contributor

@bdukes bdukes left a comment

Choose a reason for hiding this comment

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

looks good to me

Copy link
Contributor

@mitchelsellers mitchelsellers left a comment

Choose a reason for hiding this comment

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

Given the current environment, this seems to be correct. However, long-term this is a situation where the "home" for things is unclear.

@ohine
Copy link
Contributor

ohine commented Oct 28, 2018

@donker since this is an item you're actively working on, can you review/approve and merge?

@donker
Copy link
Contributor

donker commented Oct 29, 2018

I was unsure where this should go, so good catch Daniel.

@donker donker merged commit 7ad6151 into dnnsoftware:development Oct 29, 2018
Copy link
Contributor

@donker donker left a comment

Choose a reason for hiding this comment

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

I was unsure where it needed to go. This is better.

@ohine ohine added this to the 9.3.0 milestone Dec 18, 2018
@valadas valadas deleted the Issue-2430 branch December 31, 2019 06:48
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.

6 participants