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

Ensure all content models have Drupal migration metadata #729

Merged
merged 8 commits into from
Jun 24, 2023

Conversation

ErnstBas
Copy link
Contributor

@ErnstBas ErnstBas commented Jun 20, 2023

Closes #719

An abstract class has been added with the three fields to the following models:

  • community
  • documents
  • events
  • library
  • magazine
  • memorials
  • news
  • wf_pages

All classes in this files that inherit from Page also inherit from this new class DrupalFields.

I did not find other models with drupal_node_id or drupal_body_migrated.

Please let me know if this is is what you expected. As I am a beginner, feel free to request changes or reject it if this solution does not solve the issue sufficiently. Feedback to improve is very much appreciated.

Copy link
Member

@brylie brylie left a comment

Choose a reason for hiding this comment

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

In order to avoid duplicate definitions of the DrupalFields model, let's create a new common app:

python manage.py startapp common

Then, move the DrupalFields model to common/models.py and inherit from the common model in other apps.

We will also need database migrations, which are generated from the following command:

python manage.py makemigrations

Please generate the migrations and commit them to this branch.

@ErnstBas
Copy link
Contributor Author

ErnstBas commented Jun 22, 2023

Goodmorning @brylie

I did the following:

  1. Created new app common
  2. Abstract class DrupalFields moved to common/models.py
  3. Added common to INSTALLED_APPS in base.py
  4. The classes in the eight previously changed models that inherit Page also inherit DrupalFields
  5. Run python manage.py makemigrations and then python manage.py migrate

Makemigrations works, but then migrate leads to an error message:
django.db.utils.ProgrammingError: column "drupal_body_migrated" of relation "library_libraryindexpage" already exists.
I cannot see where this column was created before.

@brylie
Copy link
Member

brylie commented Jun 22, 2023

Go ahead and push your changes to GitHub, so we can review them more closely. Note that the LibraryItem is different than the LibraryIndexPage. Only the LibraryItem model should inherit from DrupalFields.

Similarly, none of the ...IndexPage models should inherit from DrupalFields. Only the following list of models should inherit from DrupalFields:

  • community: OnlineWorship
  • documents: BoardDocument, MeetingDocument
  • events: Event
  • library: LibraryItem
  • magazine: ArchiveIssue, MagazineIssue, MagazineArticle
  • memorials: Memorial
  • news: NewsItem
  • wf_pages: WfPage, MollyWingateBlogPage

@codecov
Copy link

codecov bot commented Jun 22, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.57 🎉

Comparison is base (7ffe76b) 67.76% compared to head (5e9c4b1) 68.33%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #729      +/-   ##
==========================================
+ Coverage   67.76%   68.33%   +0.57%     
==========================================
  Files          98      100       +2     
  Lines        2975     2994      +19     
==========================================
+ Hits         2016     2046      +30     
+ Misses        959      948      -11     
Impacted Files Coverage Δ
core/settings/base.py 93.42% <ø> (ø)
common/apps.py 100.00% <100.00%> (ø)
common/models.py 100.00% <100.00%> (ø)
community/models.py 100.00% <100.00%> (ø)
documents/models.py 94.23% <100.00%> (+0.11%) ⬆️
events/models.py 71.21% <100.00%> (+0.44%) ⬆️
library/models.py 64.00% <100.00%> (+0.48%) ⬆️
magazine/models.py 64.32% <100.00%> (+0.19%) ⬆️
memorials/models.py 58.82% <100.00%> (+0.82%) ⬆️
news/models.py 84.72% <100.00%> (+0.21%) ⬆️
... and 1 more

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@ErnstBas
Copy link
Contributor Author

Thanks for the heads up. Now only the list of models you have indicated inherit from DrupalFields.
I added the migration files, not sure if this was required.
In any case I continue to receive an error after running python manage.py migrate

community/models.py Outdated Show resolved Hide resolved
wf_pages/models.py Outdated Show resolved Hide resolved
@brylie
Copy link
Member

brylie commented Jun 22, 2023

I’ll check out this branch ASAP

ErnstBas and others added 2 commits June 23, 2023 08:32
Co-authored-by: Brylie Christopher Oxley <[email protected]>
Co-authored-by: Brylie Christopher Oxley <[email protected]>
wf_pages/models.py Outdated Show resolved Hide resolved
Co-authored-by: Brylie Christopher Oxley <[email protected]>
@sonarcloud
Copy link

sonarcloud bot commented Jun 24, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 11 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@codeclimate
Copy link

codeclimate bot commented Jun 24, 2023

Code Climate has analyzed commit 5e9c4b1 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (50% is the threshold).

This pull request will bring the total coverage in the repository to 68.3% (0.5% change).

View more on Code Climate.

@brylie brylie merged commit e143ac9 into WesternFriend:main Jun 24, 2023
5 checks passed
@ErnstBas ErnstBas deleted the 719 branch July 1, 2023 08:26
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.

Ensure all content models have Drupal migration metadata
2 participants