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

refactor: cmd-79 rename tacc cms settings #792

Merged
merged 51 commits into from
Jul 3, 2024

Conversation

wesleyboar
Copy link
Member

@wesleyboar wesleyboar commented Jan 31, 2024

Overview

Reword TACC_ settings as PORTAL_ settings.

Because (A) Core-CMS is used by clients, so "TACC_LOGO" is a weird setting (because they have their own logo) and (B) The CMS and Portal are aspects of a website that is called Portal in TACC-speak (and I am done grumbling against it).

Status

Related

Changes

  • renamed settings

Testing & UI

https://github.com/TACC/Core-Portal-Deployments/pull/43

@wesleyboar wesleyboar marked this pull request as draft January 31, 2024 18:39
@wesleyboar wesleyboar changed the title refactor: cmd-79 PORTAL_ not TACC_ settings refactor: cmd-79 TACC_PORTAL_ Jan 31, 2024
@wesleyboar wesleyboar marked this pull request as ready for review February 8, 2024 21:09
wesleyboar and others added 9 commits March 6, 2024 16:44
…thub.com:TACC/Core-CMS into feat/cmd-79-support-new-core-cms-settings-prefix
v4.8 was released while this was still in progress.
Fix this error
```
django_settings_export.UndefinedSettingError: "settings.FAVICON" is included in settings.SETTINGS_EXPORT but it does not exist.
```

Caused by FAVICON not used by a project, yet Core-CMS was still trying to export it.
Fix this error
```
The `LOGO` setting key is not accessible from templates: add "LOGO" to `settings.SETTINGS_EXPORT` to change that.
```

Caused by LOGO backwards compatibility neglecting SETTINGS_EXPORT.
@taoteg
Copy link
Collaborator

taoteg commented May 3, 2024

PORTAL_MANAGES_AUTH works for me, it has the PORTAL_ prefix and explicitly states what it does.
PORTAL_IS_CORE feels too ambiguous imo. Question: What property/aspect of a portal configuration is this supposed to be indicating if not that the AUTH source is the django container?

Aside: I do so which we had called Core-Portal something less overloaded (Core-Workbench, Core-Dashboard, Core-Django, Core-Workspace, literally anything else that would not conflate the use of portal), but it is what it is now...

@wesleyboar
Copy link
Member Author

I changed PORTAL_IS_CORE to PORTAL_IS_TACC_CORE_PORTAL.

Goal is clarity and accuracy. I see the setting is not solely used to describe Core-Portal managing auth. It also describes code that Core-Portal expects but is not necessary for all portals (e.g. TUP, or a future one with a better way to share header) nor related to auth.

Perhaps we revisit PORTAL_MANAGES_AUTH as a setting if we make CMS support LDAP.

@wesleyboar wesleyboar changed the title refactor: cmd-79 TACC_PORTAL_ refactor: cmd-79 rename settings Jun 28, 2024
@wesleyboar wesleyboar changed the title refactor: cmd-79 rename settings refactor: cmd-79 rename tacc cms settings Jun 28, 2024
@wesleyboar wesleyboar requested a review from taoteg July 3, 2024 22:27
@wesleyboar
Copy link
Member Author

wesleyboar commented Jul 3, 2024

I tested this on every [Camino-deployed] Core-CMS website —

— and the only bug was because of upgrading ECEP to v4.9+.

Important

Merging based on approval of https://github.com/TACC/Core-Portal-Deployments/pull/43.

@wesleyboar wesleyboar merged commit 2b15d53 into main Jul 3, 2024
@wesleyboar wesleyboar deleted the feat/cmd-79-support-new-core-cms-settings-prefix branch July 3, 2024 22:31
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.

2 participants