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

VERIFIED FIXED in Firefox 67

Status

()

enhancement
P2
minor
VERIFIED FIXED
6 months ago
17 days ago

People

(Reporter: sylvestre, Assigned: ivankhyung, Mentored)

Tracking

(Blocks 1 bug, {good-first-bug})

unspecified
Firefox 67
Points:
---

Firefox Tracking Flags

(firefox67 verified)

Details

(Whiteboard: [lang=js])

Attachments

(2 attachments)

(Reporter)

Description

6 months ago
When about:policies#active is empty, we should show a message instead of an empty list.
This is a bit confusing otherwise.
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.
Mentor: felipc
Keywords: good-first-bug
Priority: -- → P2
Whiteboard: [lang=js]
(Assignee)

Comment 2

5 months ago
I am new to contributing to Mozilla and would like to work on this bug as a way to learn how everything works.
(Assignee)

Comment 3

5 months ago
What would be a reasonable message to show? Something like: "There are no active policies."?
Flags: needinfo?(felipc)
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!
Flags: needinfo?(felipc)
(Assignee)

Comment 5

5 months ago
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.
Flags: needinfo?(felipc)
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
Flags: needinfo?(felipc)
Assignee: nobody → ivankhyung
(Assignee)

Comment 7

5 months ago
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.
Flags: needinfo?(felipc)
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.
Flags: needinfo?(felipc)
(Assignee)

Comment 9

5 months ago
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?
Flags: needinfo?(felipc)
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)
Flags: needinfo?(felipc)
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?
Flags: needinfo?(mozilla)
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.
Flags: needinfo?(mozilla)
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."
(Assignee)

Comment 14

5 months ago
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.
Flags: needinfo?(felipc)
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
Flags: needinfo?(felipc)

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

Flags: needinfo?(ivankhyung)
(Assignee)

Comment 18

2 months ago

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

Flags: needinfo?(ivankhyung)

Comment 20

2 months ago
Pushed by felipc@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c2e488c86ce
When about:policies#active is empty, show a message. r=felipe,flod

Comment 21

2 months ago
bugherder
Status: NEW → RESOLVED
Last Resolved: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 67
Flags: qe-verify+
QA Contact: emil.ghitta

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

Status: RESOLVED → VERIFIED
Flags: qe-verify+
(Reporter)

Comment 23

17 days ago

Thanks for doing that Ivan!

You need to log in before you can comment on or make changes to this bug.