Closed Bug 1146312 Opened 5 years ago Closed 4 years ago

Display partner branding and ToS/PN links on the "Get started" panel ONLY

Categories

(Hello (Loop) :: Client, defect, P3)

defect

Tracking

(firefox42 verified)

VERIFIED FIXED
mozilla42
Tracking Status
firefox42 --- verified

People

(Reporter: RT, Assigned: tomesh, Mentored)

References

Details

User Story

After discussions on bug 1113646 we agreed that we need the following change implemented to make the partner logo and ToS/PN link display more consistent with what the user expects:
- Display the partner logo and ToS/PN links on the "Get started" panel
- Do not display the partner logo and ToS/PN links on the Hello panel once "Get started" has been clicked (the partner logo should remain visible after browser restart in the Hello panel if "Get started" has not yet been clicked)

Currently the partner logo and ToS/PN links get displayed until the first call is received or until the user gets in a discussion with someone else.

Attachments

(1 file, 5 obsolete files)

No description provided.
User Story: (updated)
User Story: (updated)
Duplicate of this bug: 1147429
Rank: 37
Flags: firefox-backlog+
Priority: -- → P3
Duplicate of this bug: 1113640
From a developer perspective, we need to:

- Remove reliance on the existing loop.seenToS and the loop.showPartnerLogo preferences, so that those items are displayed all the time.
- Stop the ToSView being displayed in the rooms list.

We need to leave the standalone's use of seenToS alone for the time being.
Martin has agreed to take a run at this.
Assignee: nobody → tomesm
Mentor: standard8
I think i need a hint here. What is the meaning of "removing reliance"? looking at the code I am not able to figure out what has to be done to achieve this.

So far, after some time spending digging in the code I am (at least :) ) able to turn off ToS/PN.
(In reply to Martin Tomes [:tomesh] from comment #5)
> I think i need a hint here. What is the meaning of "removing reliance"?
> looking at the code I am not able to figure out what has to be done to
> achieve this.

Ah sorry, I should have said something like "Remove the preferences" - let the parts that they currently control be shown all the time, and remove the parts of code that set the preferences.
Attached patch pref reliance removed (obsolete) — Splinter Review
Ok this is my first tentative attempt to "remove the reliance". It is not a full patch proposal. I do not know if I understood you well.

Also Im struggling with not viewing ToSView in the "RoomsList". I have no clue how to stop viewing this. I can not see any clue in the "Room list" part of the code of panel.js
Flags: needinfo?(standard8)
Attachment #8613557 - Flags: review?(standard8)
Comment on attachment 8613557 [details] [diff] [review]
pref reliance removed

Review of attachment 8613557 [details] [diff] [review]:
-----------------------------------------------------------------

This is a good start.

For the ToS in the room list - have a look in the render function of PanelView. In the rooms tab, you'll see the <TosView /> line, which you can just remove.

Don't forget in the final version of the patch, you'll need to be making changes to panel.jsx and building the js version with the build-jsx script (https://wiki.mozilla.org/Loop/Development#Developing).

A few more comments below.

::: browser/components/loop/content/js/panel.js
@@ +276,2 @@
>          gettingStartedSeen: getPref("gettingStarted.seen"),
> +        //showPartnerLogo: getPref("showPartnerLogo")

You'll want to clean up the renderPartnerLogo function as well - I think you can merge renderPartnerLogo back into the render function for this component, obviously, we no longer need the showPartnerLogo checks.

@@ +309,1 @@
>          var terms_of_use_url = navigator.mozLoop.getLoopPref('legal.ToS_url');

When you get to tidying up, all of this will need dropping back by two spaces.

@@ +863,5 @@
>      },
>  
>      _gettingStartedSeen: function() {
>        this.setState({
> +        gettingStartedSeen: this.props.mozLoop.getLoopPref("gettingStarted.seen"),

nit: unnecessary change.
Attachment #8613557 - Flags: review?(standard8) → feedback+
Flags: needinfo?(standard8)
Attached patch <ToSView /> removed (obsolete) — Splinter Review
Well, after removing <ToSView /> the ToS and PN is not showing at all which is not the required behavior Im afraid :( 

I did not touch Logo rendering yet (this can be done later), cause it is not important for me at the moment. I need to somehow get the required behavior. Any suggestions how to keep ToS and PN until get started is clicked? So far I have followed the suggested solution: 
1) removed reliance on preferences
2) removed <ToSView /> to not being seen.

moreover, for some reason, after running jsx compiler some other files has been re-generated, so the patch is a bit messy. sorry for that (I did not expect this).

PS: sorry for delayed reactions, Im quite busy at work at the moment.
Attachment #8617899 - Flags: review?(standard8)
Comment on attachment 8617899 [details] [diff] [review]
<ToSView /> removed

Review of attachment 8617899 [details] [diff] [review]:
-----------------------------------------------------------------

Hi Martin, no problems with delays. This is heading in the right direction. I've added a few more comments below which will hopefully make sense.

Re the issues with the generated files - I'm not quite sure what's up there. I would suggest unapplying the patch, pulling the latest fx-team repository, if you're doing it via hg, also check if there's anything listed in `hg outgoing`. If there is, then you might have updated over a patch or something else. `hg strip` can strip out individual changesets, but it depends if you've got multiple heads or not.

If you send me the output of `hg outgoing` (or chat on irc), then I might be able to help further.

::: browser/components/loop/content/js/panel.jsx
@@ -273,5 @@
>  
>        return {
> -        seenToS: getPref("seenToS"),
> -        gettingStartedSeen: getPref("gettingStarted.seen"),
> -        showPartnerLogo: getPref("showPartnerLogo")

In renderPartnerLogo, there's a check for showPartnerLogo as well, so you'll need to remove that and the setLoopPref call.

@@ -923,5 @@
>            <div>
>              <NotificationListView notifications={this.props.notifications}
>                                    clearOnDocumentHidden={true} />
>              <GettingStartedView />
> -            <ToSView />

You need to keep this line - this is where we want the ToS displayed.

@@ -949,5 @@
>                <RoomList dispatcher={this.props.dispatcher}
>                          store={this.props.roomStore}
>                          userDisplayName={this._getUserDisplayName()}
>                          mozLoop={this.props.mozLoop}/>
> -              <ToSView />

This was the right one to remove.
Attachment #8617899 - Flags: review?(standard8) → feedback+
still a tentative patch. the test case will be next step if this changes are acceptable :)
Attachment #8623633 - Flags: review?(standard8)
Comment on attachment 8613557 [details] [diff] [review]
pref reliance removed

Need to remember to obsolete old patches, this was getting me confused using automated download tools ;-)
Attachment #8613557 - Attachment is obsolete: true
Attachment #8617899 - Attachment is obsolete: true
Comment on attachment 8623633 [details] [diff] [review]
Requested customization WITHOUT testing.

Review of attachment 8623633 [details] [diff] [review]:
-----------------------------------------------------------------

This is great! The panel behaves just right with this patch.

I just remembered, whilst we're looking at code, there's a few more instances of pref setting/checking to tidy up:

http://hg.mozilla.org/mozilla-central/annotate/d7c148c84594/browser/components/loop/modules/LoopCalls.jsm#l213
http://hg.mozilla.org/mozilla-central/annotate/d7c148c84594/browser/components/loop/content/shared/js/activeRoomStore.js#l725
http://hg.mozilla.org/mozilla-central/annotate/d7c148c84594/browser/components/loop/standalone/content/js/standaloneMozLoop.js#l273

Those should be fairly easy to remove.

::: browser/components/loop/content/js/panel.jsx
@@ +284,2 @@
>      render: function() {
>        if (!this.state.gettingStartedSeen || this.state.seenToS == "unseen") {

Can you remove the `|| this.state.seenToS == "unseen"` section of this if statement as well please.
Attachment #8623633 - Flags: review?(standard8) → feedback+
Attached patch cleaning preferences chekcing (obsolete) — Splinter Review
All references to preferences should be cleared. What is the next step now? An unit test?
Attachment #8623633 - Attachment is obsolete: true
Attachment #8626515 - Flags: review?(standard8)
Comment on attachment 8626515 [details] [diff] [review]
cleaning preferences chekcing

Review of attachment 8626515 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great!

Yes, tests are next. I would suggest attempting to run the ./browser/components/loop/run-all-loop-tests.sh script next (run it from the root directory of mozilla-central). This should start to show you where existing tests fail - its highly probable that most of the affected tests can just be removed, although I suspect some may need adjusting.

The tests run in the order of eslint, xpcshell, marionette unit tests, mochitests. For the marionette unit tests, you can load them on a normal web page in the browser - see https://wiki.mozilla.org/Loop/Development#Visiting_test_pages for details.
Attachment #8626515 - Flags: review?(standard8) → feedback+
Attached patch Test cases customize (obsolete) — Splinter Review
I have finally got to customizing the test cases. Here is my first attempt. For now I have removed all unnecessary tests of preference settings. But for some reason I have failed test for rendering telefonica logo. It seems that it is not rendering at all and I do not know why. I would appreciate any hint here. Thanks.
Attachment #8626515 - Attachment is obsolete: true
Attachment #8642305 - Flags: review?(standard8)
Great, thanks for the update!

I see why the failing test confused you - it was rendering the image, but the actual test was wrong. Additionally the description for the test was misleading as well.

I debated about another round of comments & changes but decided the patch was close enough that it was close enough to get it finished. Here's what I've changed:

- Given the functionality changes to the ToSView I initially simplified it to two tests - based on the value of loop.gettingStarted.seen was the view shown or not.
- However, I then realised that there must be a switch somewhere else in another view for that - it turns out that the parent view (PanelView) already has the !this.state.gettingStartedSeen check, hence, ToSView no longer needs the gettingStartedSeen, and hence I removed that code.
- Finally, the tests for ToSView were removed, and I realised that the PanelView tests needed extending slightly, to check that the ToSView was displayed when gettingStartedSeen was false.

Hence, I've done the changes as they were relatively simple, and here's the new patch. Hope that's ok, if you can, take a look before I land it and check it still looks reasonable for you!
Attachment #8642305 - Attachment is obsolete: true
Attachment #8642305 - Flags: review?(standard8)
Attachment #8642452 - Flags: feedback?(tomesm)
Attachment #8642452 - Flags: review+
Comment on attachment 8642452 [details] [diff] [review]
Display partner branding and ToS/PN links on the GetStarted only.

Hi Martin, I decided to push this to avoid it getting bitrotted with other patches we're doing in the same area.

Let me know if there's anything I missed, I did test it several times to make sure it did the right thing.

Thanks for all your work on this, and congratulations.
Attachment #8642452 - Flags: feedback?(tomesm)
Flags: qe-verify+
Hi Mark, 

I went through your update and it make sense for me. I can see anything missed. I'm glad that I could help... despite not being as fast as I would like to (you know It's lazy summer :) ).
https://hg.mozilla.org/mozilla-central/rev/f9a99beaf0cd
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
I can confirm that partner branding and ToS/PN are only displayed in 'Get Started' panel using Firefox 42 beta 4 across platforms (Windows 7 64-bit, Mac OS X 10.10.5 and Ubuntu 14.04 32-bit).
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.