-
Notifications
You must be signed in to change notification settings - Fork 0
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
feat(outlook): generate plugin xml manifest #208
Conversation
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.
looks good, just one minor change
src/controllers/outlook.cr
Outdated
|
||
get("/manifest.xml") do | ||
manifest = OutlookManifest.new( | ||
app_domain: "#{App::PLACE_URI}/outlook/", |
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.
we don't want to use App::PLACE_URI
this is for internal comms between the containers
we need to use the current tenant.domain
and only if tenant.platform == "office365"
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.
Ok, can we get the tenant domain and platform without auth?
There is a requirement for the xml manifest to be served without auth.
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 guess we could make the tenant part of the path, or a query param
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.
it doesn't need auth, just can't use the helper (I wasn't aware the helper had the extra check either)
domain_host = request.hostname.as(String)
tenant = Tenant.query.find { domain == domain_host }
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.
LGTM, let's hold off merging until the XML is finalised
@jeremyw24 to notify of any further changes required
@jeremyw24 any additional XML changes now that the office plugin is working? |
@stakach
|
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.
Awesome work, looking good!
No description provided.