SF-3708 Allow email sharing only on main host#3685
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3685 +/- ##
=======================================
Coverage 81.84% 81.84%
=======================================
Files 618 619 +1
Lines 38603 38608 +5
Branches 6290 6314 +24
=======================================
+ Hits 31594 31600 +6
+ Misses 6048 6047 -1
Partials 961 961 ☔ View full report in Codecov by Sentry. |
Nateowami
left a comment
There was a problem hiding this comment.
@Nateowami reviewed 1 file and made 2 comments.
Reviewable status: 1 of 4 files reviewed, 2 unresolved discussions (waiting on @RaymondLuong3).
src/SIL.XForge.Scripture/ClientApp/src/app/shared/share/share-control.component.ts line 72 at r1 (raw file):
readonly invalidEmailAddress: string = 'invalid-email-address'; private readonly hostname: string = this.locationService.hostname;
I can't think of a good reason to cache this value on the component instead of just reading it directly from the service.
src/SIL.XForge.Scripture/ClientApp/src/app/shared/share/share-control.component.ts line 125 at r1 (raw file):
get showEmailInvite(): boolean { return !this.hostname.includes('scribdocs');
This is not the way. We should have a utility function somewhere to check if on the whitelabeled site, and it should use similar logic as RazorPageSettings.cs.
For example:
const isMainSite = this.locationService.host === new URL(environment.masterUrl).hostYou will not find the name of any white labeled version anywhere in the repo, and that is by design.
2e341e8 to
fb5c980
Compare
RaymondLuong3
left a comment
There was a problem hiding this comment.
@RaymondLuong3 made 2 comments.
Reviewable status: 0 of 5 files reviewed, 2 unresolved discussions (waiting on @Nateowami).
src/SIL.XForge.Scripture/ClientApp/src/app/shared/share/share-control.component.ts line 72 at r1 (raw file):
Previously, Nateowami wrote…
I can't think of a good reason to cache this value on the component instead of just reading it directly from the service.
Done.
src/SIL.XForge.Scripture/ClientApp/src/app/shared/share/share-control.component.ts line 125 at r1 (raw file):
Previously, Nateowami wrote…
This is not the way. We should have a utility function somewhere to check if on the whitelabeled site, and it should use similar logic as RazorPageSettings.cs.
For example:
const isMainSite = this.locationService.host === new URL(environment.masterUrl).hostYou will not find the name of any white labeled version anywhere in the repo, and that is by design.
Good idea! I had not thought of using the environment, but that makes a lot of sense.
Nateowami
left a comment
There was a problem hiding this comment.
@Nateowami reviewed 5 files and all commit messages, made 3 comments, and resolved 2 discussions.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @RaymondLuong3).
src/SIL.XForge.Scripture/ClientApp/src/app/shared/share/share-control.component.ts line 125 at r1 (raw file):
Previously, RaymondLuong3 (Raymond Luong) wrote…
Good idea! I had not thought of using the environment, but that makes a lot of sense.
Look good; thanks. And I'm sorry if I was too blunt with my earlier response. One goal with the white-labeled site is to avoid publishing anything that can draw a connection from the public version to it. Also, we may have multiple white-labeled sites in the future, so any checks should be looking to see if it's the main site, not a particular white-labeled version.
src/SIL.XForge.Scripture/ClientApp/src/app/core/branding.service.ts line 10 at r2 (raw file):
get useScriptureForgeBranding(): boolean { return this.locationService.host !== new URL(environment.masterUrl).host;
This logic is inverted
src/SIL.XForge.Scripture/ClientApp/src/app/shared/share/share-control.component.html line 8 at r2 (raw file):
} <div class="invite-forms"> @if (showEmailInvite) {
Could we move this below the share a link option? I've felt for a long time that the order here is wrong, and now that one of them is only conditionally shown, it seems to me to make more sense to have it come last. I realize this wasn't part of the original scope.
RaymondLuong3
left a comment
There was a problem hiding this comment.
@RaymondLuong3 made 3 comments.
Reviewable status: 3 of 5 files reviewed, 2 unresolved discussions (waiting on @Nateowami).
src/SIL.XForge.Scripture/ClientApp/src/app/core/branding.service.ts line 10 at r2 (raw file):
Previously, Nateowami wrote…
This logic is inverted
Done.
src/SIL.XForge.Scripture/ClientApp/src/app/shared/share/share-control.component.html line 8 at r2 (raw file):
Previously, Nateowami wrote…
Could we move this below the share a link option? I've felt for a long time that the order here is wrong, and now that one of them is only conditionally shown, it seems to me to make more sense to have it come last. I realize this wasn't part of the original scope.
src/SIL.XForge.Scripture/ClientApp/src/app/shared/share/share-control.component.ts line 125 at r1 (raw file):
Previously, Nateowami wrote…
Look good; thanks. And I'm sorry if I was too blunt with my earlier response. One goal with the white-labeled site is to avoid publishing anything that can draw a connection from the public version to it. Also, we may have multiple white-labeled sites in the future, so any checks should be looking to see if it's the main site, not a particular white-labeled version.
No worries. All good!
Nateowami
left a comment
There was a problem hiding this comment.
@Nateowami made 2 comments and resolved 2 discussions.
Reviewable status: 3 of 5 files reviewed, all discussions resolved (waiting on @RaymondLuong3).
src/SIL.XForge.Scripture/ClientApp/src/app/shared/share/share-control.component.html line 8 at r2 (raw file):
Thanks
|
By the way, @RaymondLuong3 the way to view the whitelabeled site locally is to go to http://127.0.0.1:5000/ |

This PR hides the invite by email option if the administrator is using the white label site.
Before

After

This change is