-
Notifications
You must be signed in to change notification settings - Fork 2
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
Add draft of schema file #2
Conversation
cc @freaktechnik & @vDeo Do you two have any questions, or concerns about this spec? |
I've got a more fields to add (thankfully, @Sancus alerted me to the missing fields), but won't be able to address until tomorrow |
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.
I just had a thought: we can handle localization with the locale filtering, so we have one of these objects per locale and just filter them.
schema.json
Outdated
"start_at": { | ||
"type": "string", | ||
"format": "date-time", | ||
"description": "Timestamp after which Thunderbird will show the event after startup." | ||
}, | ||
"end_at": { | ||
"type": "string", | ||
"format": "date-time", | ||
"description": "Timestamp after which Thunderbird will never show the event, even if it has never been shown to the user." | ||
}, |
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.
I assume these will be ...T...Z
timestamps? From the json schema docs I could find they didn't specify that it should include a timezone.
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.
I think that including a timezone should be fine?
I couldn't find a validator that actually checked if a date-time
string field was formatted correctly.
(The validators said everything was 👍 whether I put in a timestamp with/without a timezone...and even the string "this is not a timestamp"
)
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.
This should always be UTC for consistency when we are publishing events. It's fine if the client is able to handle other time zones, but not required.
schema.json
Outdated
}, | ||
"description": { | ||
"type": "string", | ||
"description": "A short paragraph that can contain HTML and will be displayed in the Thunderbird UI." |
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.
I think we should limit the allowed HTML so we can sanitize it and display it confidently. Obviously we can do this unilaterally in the client anyway, but maybe we'd want to define that somewhere.
.
@MelissaAutumn suggested something similar: maybe when the client requests the notifications, it sends a locale string as a parameter. That way, the server only sends back the locale-specific notifications. |
schema.json
Outdated
"exclude": { | ||
"$ref": "#/definitions/profile" | ||
}, | ||
"include": { | ||
"$ref": "#/definitions/profile" | ||
} |
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.
Should these be arrays of profile in case we want to target specific combinations? I don't love the arrays of arrays structure this would lead to though.
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.
🤔 making them arrays of profile
is probably the best way to do combinations. But yeah, could be annoying to process on the client.
Right, but that would mean you're doing actual work on the server side. As I had mentioned, we should easily be able to do an URL similar to what the release notes have: https://searchfox.org/comm-central/rev/c04464d92274c9d2b8ec036d47b0c664adf1a45f/mail/app/profile/all-thunderbird.js#127 |
schema.json
Outdated
"channels": { | ||
"type": "array", | ||
"items": { | ||
"enum": ["esr", "release", "beta", "daily"], |
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.
I think this would be default
for builds that aren't on any channel, like local builds or possibly other "unbranded" builds. The SoW doesn't have any mention of that, just trying to think of all the permutations of what we might encounter in the wild.
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.
That's a good point. @Sancus, any objection to sending notifications to any/all builds by specifying default
?
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.
Not sure it was clear, I meant the string default as enum value, so "default"
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.
E: Nevermind all that stuff. On further examination, there is actually a decent number of folks on the default
channel, so yeah it makes sense to include that here. I'm guessing this is used by some distros for their package managed builds. It might be useful to target.
There ARE a lot of other minor channels but I don't think we need targeting for them in v1.
To target everybody NOT on a major channel I would still go with a blacklist.
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.
👍
Question: Should I add a "denylist" to the schema? (and maybe change "targeting" to "allowlist"?)
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.
Isn't that what exclude
and include
do?
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.
hahahaha yes.
😂
schema.json
Outdated
"type": "array", | ||
"items": { | ||
"enum": [ | ||
"windows", |
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.
I think these values should probably be the ones used by Thunderbird itself. That was my intent for all the targeting filters, that they mirror the values used internally in the client so Thunderbird doesn't need some crazy remapping scheme in comm-central
.
I'm not sure what all possible values of %OS%
are in Thunderbird. I think the major three are WINNT
, Darwin
, and Linux
though. @jfx2006 might know or know where you can find lists of these things.
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.
I've been looking at https://searchfox.org/mozilla-central/source/toolkit/modules/AppConstants.sys.mjs since that's what I get at runtime.
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.
Ah, the URL formatter uses appinfo
: https://searchfox.org/mozilla-central/source/toolkit/components/urlformatter/URLFormatter.sys.mjs#71-136, which uses OS_TARGET
: https://searchfox.org/mozilla-central/source/toolkit/xre/nsAppRunner.cpp#1217, so that'd be the values you're talking about.
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.
I've been looking at https://searchfox.org/mozilla-central/source/toolkit/modules/AppConstants.sys.mjs since that's what I get at runtime.
Ah, if it's easy for you to get that at runtime then nevermind, use that. Seems fine to me.
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.
If we'd want to match it that'd still mean "win"
and "macosx"
as values though.
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.
Yeah definitely match, it will avoid having to maintain mappings on the client side.
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.
👍 makes sense! I've updated those two enum values to match AppConstants.sys.mjs
@freaktechnik @Sancus The changes introduced by #5 include a command for generating a JSON schema from a pydantic model. The pydantic model includes all the same enum values and constraints as the JSON schema in this PR. Would it be reasonable to close this PR in favor of a generated schema file? |
Hey @radishmouse can you commit a current copy of that schema to repo for reference? And a general note it is missing the dependantRequired key (#7) but is otherwise matching. |
@MelissaAutumn generated schema added to this PR Also, I updated the PR description making it clear that the generated schema is missing the |
I assume all the optional keys being nullable is a side effect of what it's being generated from? I think the manually written schema currently captures some less formal conventions on the interface, which the generated schema doesn't document. |
It sounds like the manually written schema is preferred. I'll merge this PR and update the JSON validation GH action to use it. |
Closes #1
The new
schema.json
includes the fields mentioned in the tech spec as well as:Please let me know if anything is missing or needs improvement!
Update: now includes a version of the schema generated by the pydantic model (from #5).
With the exception of the
dependantRequired
key , it is equivalent to the schema.json introduced by this PR.