-
Notifications
You must be signed in to change notification settings - Fork 84
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
chore: remove custom logo #979
Conversation
// Check number of forms with customLogo key | ||
db.getCollection('forms').find({ customLogo: { $exists: true } }).count() | ||
|
||
// Check number of forms with startPage.logo key | ||
db.getCollection('forms').find({ "startPage.logo.state": { $exists: true } }).count() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we also check how many unarchived forms have only the customLogo
key but not startPage.logo.state
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on my queries yesterday:
Total Number of forms: 179888
Forms with startPage.logo.state
defined: 139912
Breaks down into:
DEFAULT: 112081
NONE: 8297
CUSTOM: 19534
customLogo
may exist for such forms too, but is currently ignored as startPage.logo
is defined
Forms with startPage.logo.state
undefined: 39977
Breaks down into:
customLogo
undefined: 36359 => Currently defaults to agency logo
customLogo
defined: 3618 => Currently defaults to customLogo
, but once the key is removed, will default to agency logo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@liangyuanruo there are a sizeable number of forms (i.e. 36359) with neither startPage.logo.state
nor customLogo
as new forms are created without either key
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to clarify, this is because it's possible to have a form without a logo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Old Code
- If
startPage.logo.state
present, will use state defined therein - If
startPage.logo.state
absent, will use state defined incustomLogo
. Note thatundefined
state forcustomLogo
means that the default agency logo is to be shown - New forms are created with
startPage.logo.state
absent andcustomLogo
undefined
and hence show the default agency logo
New Code
- If
startPage.logo.state
present, will use state defined therein - Will ignore
customLogo
, althoughcustomLogo
should have been removed fromform
documents - If
startPage.logo.state
absent, default agency logo will be shown - New forms are created with
startPage.logo.state
absent and hence show the default agency logo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does that make sense @liangyuanruo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Always happy to see a PR removing code. Generally LGTM but could I request expanding on the DB script so that we can be 100% sure that the customLogo
key is safe to remove?
how would you suggest I expand it? @liangyuanruo |
Problem
Closes #787
Solution
form.customLogo
which was no longer being usedform.hasheader
which was no longer being usedform.logo
which was no longer being usedform.customLogo
Notes
form.customLogo
has been deleted, such forms with either default to the config instartPage.logo
or ifstartPage.logo
is non-existent (i.e. for all new forms), default to the agency logo.Tests
Deploy Notes