1497934 - When about:policies#active is empty, show a message

Status: VERIFIED FIXED

(Firefox :: Enterprise Policies, enhancement, P2)

Description

[Sylvestre Ledru [:Sylvestre]]

When about:policies#active is empty, we should show a message instead of an empty list.
This is a bit confusing otherwise.

Comment 1

[:Felipe Gomes (needinfo for replies!)]

Bug 1485196 is meant as a better solution for that, but I won't dupe it because we can use your suggestion as a quicker fix for this empty UI which does look confusing.

Comment 2

[Ivan Yung]

I am new to contributing to Mozilla and would like to work on this bug as a way to learn how everything works.

Comment 3

[Ivan Yung]

What would be a reasonable message to show? Something like: "There are no active policies."?

Comment 4

[:Felipe Gomes (needinfo for replies!)]

Hello! Thanks for volunteering!


First, to get started, you'll need to check out a copy of the Firefox source code from Mercurial (https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Source_Code/Mercurial), and then set up the build environment to build Firefox.

Since to fix this bug it won't be necessary to recompile any binary components for Firefox, I recommend setting up an artifact build which is much faster and easier to use: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Artifact_builds


Then, to actually fix the bug, you should look into the aboutPolicies.js file (https://searchfox.org/mozilla-central/source/browser/components/enterprisepolicies/content/aboutPolicies.js), specially the generateActivePolicies function.


We actually need to different messages here:

- one for the case that the policy service is active, but no policies were specified
- another for the case that the policy service is inactive (which will be the most common case)


We can decide on the actual text for these messages later, once you get started!

Comment 5

[Ivan Yung]

I created a build environment and have taken a look at aboutPolicies.js.

However, I am a little confused about the case when:

> the policy service is active, but no policies were specified

From my understanding of Enterprise Policies, the policies are taken from a policies.json file. When the file is empty/there is no file, there will be no policies. When the json file has a policy in it, it will be active. So I am unsure how to have no policies specified.

Comment 6

[:Felipe Gomes (needinfo for replies!)]

It could be that the file is present (thus activating the policy engine), but in the contents of the file, no policy was specified. e.g:

{ policies: [] }

You can tell if the engine is active by checking Services.policies.status == Services.policies.ACTIVE

Comment 7

[Ivan Yung]

I have added an if statement at the end of the generateActivePolicies function. The if statement checks if the policy service is not active and will generate a message if it isn't. If the service is active, the if statement checks how many policies were counted and will output a different message if there are zero policies. My code looks like:

if (Services.policies.status != Services.policies.ACTIVE) {
    //message that policy service is inactive
} else if (policy_count < 1) {
    //message that policy service is active but there are no specified policies
}

Assuming that this is fine, I would like to know what would be a good way to display these messages. I could try to place text in the centre or place the message text where the column headers are. I would appreciate your input.

Comment 8

[:Felipe Gomes (needinfo for replies!)]

Yeah, that's definitely fine!

I think we should completely hide the table, and add an <h3> or <p> with the text message, aligned with the "Active" label.

Comment 9

[Ivan Yung]

I now hide the table by setting its display value to none from the Javascript. I then create a <h3> element and add it to the active tab. The code looks like:

if (policy_count < 1) {
    let active_tab = document.getElementById("active");
    active_tab.getElementsByTagName("table")[0].style.display = "none";
    let message = document.createElement("h3");
    let message_text = document.createTextNode(Services.policies.status == Services.policies.ACTIVE ? message1 : message2);
    message.style.padding = "1rem";
    message.appendChild(message_text);
    active_tab.appendChild(message);
}

Currently my messages are "The policy service is active but there are no specified policies." and "The policy service is inactive.". Would these messages work?

Comment 10

[:Felipe Gomes (needinfo for replies!)]

Yeah, the idea on the code is exactly right. I'd just prefer to make this as much through the CSS file (aboutPolicies.css) as possible.

So you can add the 1rem padding there, but you could also make the table/h3 visible through a CSS class, then in your JS code you just set that class (on the active_tab div) accordingly.

Then you can have the <h3> directly in the html file instead of having to create them in JS. This will help with the strings because we need to make them localizable. Take a look at how that works by looking at other examples in the file: basically there's a "data-l10n-id" attribute that specifies the id for the string, and the actual strings are added in the aboutPolicies.ftl file.


As for the messages, they sound good to me with one modification: The policy service -> The Enterprise Policies service.

(but I'll double check with mkaply about what he thinks about the text of the messages)

Comment 11

[:Felipe Gomes (needinfo for replies!)]

Hey Mike, we're fine tuning the message to display in about:policies if there are no active policies. How do they sound to you?


-> "The Enterprise Policies service is active but there are no specified policies."

and

-> "The Enterprise Policies service is inactive."


I'm specially wondered about the use of "service" as I don't think we used that anywhere else. But it sounds appropriate, so maybe we should start using it?

Comment 12

[Mike Kaply [:mkaply]]

This is fine with me. For the first message, it would read better as:

The Enterprise Policies service is active but there are no policies enabled.

Comment 13

[:Felipe Gomes (needinfo for replies!)]

Thanks! Ivan, we're good to go with:

"The Enterprise Policies service is active but there are no policies enabled."

and

"The Enterprise Policies service is inactive."

Comment 14

[Ivan Yung]

Sorry for the delay!

I have changed my code so I do most my work in the css. I have added two <h3> messages that are shown based on what other class is on the active_tab div. I also hide the table based on the classes in the active_tab div.

In <div id="active" class="tab active">, I have added:

<h3 class="inactive-service-message" data-l10n-id="inactive-message"></h3>
<h3 class="no-specified-policies-message" data-l10n-id="no-specified-policies-message"></h3>

In aboutPolicies.css, I have added:

.inactive-service > table, .no-specified-policies > table {
  display: none;
}
.inactive-service > .inactive-service-message {
  display: inline;
}
.no-specified-policies > .no-specified-policies-message {
  display: inline;
}
.tab > h3 {
  padding: 1rem;
  display: none;
}

My js code is now:

if (policy_count < 1) {
    let current_tab = document.querySelector(".active");
    if (Services.policies.status == Services.policies.ACTIVE) {
      current_tab.classList.add("no-specified-policies");
    } else {
      current_tab.classList.add("inactive-service");
    }
}

I initially tried using only one <h3> and using js to change its "data-l10n-id" attribute to display the correct message. However, I found that the message would fail to change when about:policies initially loaded. It would only change when I refreshed the page.

Comment 15

[:Felipe Gomes (needinfo for replies!)]

Thanks, that looks great. Do you know how to submit a patch?

The easiest way is for you to generate a diff of these files (using `hg diff`) and attaching the resulting file here on this bug.

The other way is to use the new phabricator tool that we're using. It's a bit more complicated to set up, but once set up it's easy.. If you want to read about it, take a look here: https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html

Comment 16

[Ivan Yung]

Attachment: Bug 1497934 - Created 2 messages for About Policies' Active Tab, one for no active policies and one for inactive policy service.

Comment 17

[:Felipe Gomes (needinfo for replies!)]

Hello Ivan, did you see my review comment in Phabricator?

Comment 18

[Ivan Yung]

Sorry, I just saw it yesterday! I'll get it done when I have time.

Comment 19

[Ivan Yung]

Attachment: Bug 1497934 - Simplifying CSS for 2 messages in About Policies' Active Tab

Depends on D14022

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c2e488c86ce
When about:policies#active is empty, show a message. r=felipe,flod

Comment 21

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/2c2e488c86ce

Comment 22

[Emil Ghitta, QA [:emilghitta]]

This is verified fixed using Firefox 67.0b6 (BuildId:20190328152334) on Windows 10 64bit, macOS 10.12.6 and Ubuntu 16.04 32bit.

Comment 23

[Sylvestre Ledru [:Sylvestre]]

Thanks for doing that Ivan!