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

Remove public site #3738

Merged
merged 15 commits into from
Feb 6, 2024
Merged

Remove public site #3738

merged 15 commits into from
Feb 6, 2024

Conversation

theskumar
Copy link
Member

@theskumar theskumar commented Jan 29, 2024

Description

  • Keeps the migrations to delete the public tables
  • Updates and adds a global footer (admin configurable)
  • Update the layout of homepage

Fixes #3110 #3559

Test Steps

  • Remove the public site, go to "Wagtail Admin" > "Pages", set the "Apply Home Page" as the default page of the site. Remove the public site page
  • Do regression of the whole site

@theskumar theskumar force-pushed the maintainance/public-base-html branch from 8e890a0 to f4d0ee2 Compare January 29, 2024 12:59
@theskumar theskumar marked this pull request as ready for review January 29, 2024 14:10
@frjo
Copy link
Member

frjo commented Jan 30, 2024

I have merged in all "Remove public…" PRs before this one now so this can be rebased of main.

@theskumar theskumar force-pushed the maintainance/svg-updates branch from b579c86 to d032b7c Compare January 30, 2024 07:05
@theskumar theskumar force-pushed the maintainance/public-base-html branch 2 times, most recently from ea88722 to 93bf99a Compare January 30, 2024 07:44
Base automatically changed from maintainance/svg-updates to main January 30, 2024 08:25
@frjo
Copy link
Member

frjo commented Jan 30, 2024

I merged in the icon PR so this can be rebased.

I also see this error:

WARNINGS:
?: (urls.W003) Your URL pattern 'account/two_factor/setup/complete/' [name='two_factor:setup_complete'] has a name including a ':'. Remove the colon, to avoid ambiguous namespace references.
?: (urls.W005) URL namespace 'apply:projects:reports' isn't unique. You may not be able to reverse all URLs in this namespace

@frjo
Copy link
Member

frjo commented Jan 30, 2024

So nice seeing all the delete file statements in this PR!

@theskumar theskumar force-pushed the maintainance/public-base-html branch 4 times, most recently from be57656 to 7a5603f Compare January 30, 2024 08:57
Remove public site

- Keeps the migrations to delete the public tables
- Updates and adds a global footer (admin configurable)
- Update the layout of homepage
- To succesfully remove the public site, go to "Wagtail Admin" >
  "Pages", set the "Apply Home Page" as the default page of the site.
Remove the public site page

Fixup: update heaader nav
@theskumar theskumar force-pushed the maintainance/public-base-html branch from 7a5603f to 154585b Compare January 30, 2024 08:59
@theskumar theskumar force-pushed the maintainance/public-base-html branch from 70159fa to c2a69d9 Compare January 30, 2024 09:13
@theskumar
Copy link
Member Author

I merged in the icon PR so this can be rebased.

I also see this error:

WARNINGS:
?: (urls.W003) Your URL pattern 'account/two_factor/setup/complete/' [name='two_factor:setup_complete'] has a name including a ':'. Remove the colon, to avoid ambiguous namespace references.
?: (urls.W005) URL namespace 'apply:projects:reports' isn't unique. You may not be able to reverse all URLs in this namespace

These two warning doesn't seem to b/c of changes in the PR. If it make sense I'll update them in this PR or have them resolved in another PR.

@theskumar theskumar force-pushed the maintainance/public-base-html branch from 1c6bf74 to c2a69d9 Compare January 30, 2024 09:45
@theskumar
Copy link
Member Author

Unable to get the failing test reproduced locally. Will try it tomorrow.

@theskumar
Copy link
Member Author

Django did had start up issues in very old versions before they introduced the apps.py config option with ready() function in that would be excecuted after all the model classes are loaded. So that could be it.

@theskumar
Copy link
Member Author

@wes-otf you saved me hours. The solutions seems to be working and accessing the models before its initialized is a very bad idea in general. The loading of models and apps are very unpredictable.

That why Django introduced https://docs.djangoproject.com/en/5.0/ref/applications/#django.apps.AppConfig.ready to you can perform any on_load activities here.

I believe there are more code in the forms.py in certain apps that is initialized like this, instead of in the __init__.py looks like the fixes things for now. But we can re-visit and cleap all module level queries.

@frjo
Copy link
Member

frjo commented Feb 5, 2024

This doc need updating at some point as well:

### Reversing URLS
Due to the way the urls are [configured](https://github.com/HyphaApp/meta/tree/6060bb2491e501e501979a68036dd52f60f6a6fe/Contributing/Implementation.md#url-configuration), tests for the Apply site will fail as the Apply site does not use the default URL config.
This can be resolved on a per testcase basis using the following:
```python
from django.test import TestCase, override_settings
@override_settings(ROOT_URLCONF='hypha.apply.urls')
class MyTestCase(TestCase):
pass
```
This is implemented by default for the `BaseViewTestCase`

@theskumar
Copy link
Member Author

Reversing URLS

Updated.

@theskumar
Copy link
Member Author

Looking into the test case failing. Let's hold on to merging before that. This something to do with the updated Sites setting.

@frjo
Copy link
Member

frjo commented Feb 5, 2024

Can it be that the initial migrations created a public and apply site and set the public as default. When testing against this we get errors.

@wes-otf
Copy link
Contributor

wes-otf commented Feb 5, 2024

@theskumar glad it worked!! Was a bit of a shot in the dark, weird stuff.

Update migration to use the apply homepage as default
@theskumar
Copy link
Member Author

Can it be that the initial migrations created a public and apply site and set the public as default. When testing against this we get errors.

Yes. You are absolutely right. updated the code in b788eec

Re-created by creating a fresh db, ran migration, without adding the sandbox db data

@theskumar theskumar force-pushed the maintainance/public-base-html branch from b788eec to 259948d Compare February 6, 2024 06:39
@frjo
Copy link
Member

frjo commented Feb 6, 2024

Deploying this to test now.

@frjo frjo temporarily deployed to test-hypha-app February 6, 2024 10:59 Inactive
@frjo
Copy link
Member

frjo commented Feb 6, 2024

I have set the apply site as default on test now.

Skärmavbild 2024-02-06 kl  12 57 20

@frjo frjo added Type: Maintenance Type: Minor Minor change, used in release drafter Status: Needs user testing 👷 Tasks that should be tested by OTF's user test team and removed Status: Needs user testing 👷 Tasks that should be tested by OTF's user test team labels Feb 6, 2024
@frjo frjo merged commit 2db6a27 into main Feb 6, 2024
@theskumar theskumar deleted the maintainance/public-base-html branch February 7, 2024 10:09
wes-otf added a commit that referenced this pull request May 7, 2024
- Keeps the migrations to delete the public tables
- Updates and adds a global footer (admin configurable)
- Update the layout of homepage

Fixes #3110 #3559


## Test Steps

- [ ] Remove the public site, go to "Wagtail Admin" > "Pages", set the
"Apply Home Page" as the default page of the site. Remove the public
site page
 - [ ] Do regression of the whole site

---------

Co-authored-by: Wes Appler <[email protected]>
wes-otf added a commit that referenced this pull request May 8, 2024
- Keeps the migrations to delete the public tables
- Updates and adds a global footer (admin configurable)
- Update the layout of homepage

Fixes #3110 #3559


## Test Steps

- [ ] Remove the public site, go to "Wagtail Admin" > "Pages", set the
"Apply Home Page" as the default page of the site. Remove the public
site page
 - [ ] Do regression of the whole site

---------

Co-authored-by: Wes Appler <[email protected]>
Vldln pushed a commit to equalitie/hypha that referenced this pull request May 28, 2024
- Keeps the migrations to delete the public tables
- Updates and adds a global footer (admin configurable)
- Update the layout of homepage

Fixes HyphaApp#3110 HyphaApp#3559

- [ ] Remove the public site, go to "Wagtail Admin" > "Pages", set the
"Apply Home Page" as the default page of the site. Remove the public
site page
 - [ ] Do regression of the whole site

---------

Co-authored-by: Wes Appler <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Maintenance Type: Minor Minor change, used in release drafter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants