Skip to content
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

[bug] Hide empty usage box when RadiusGroup has no checks #819 #845

Closed
wants to merge 6 commits into from
5 changes: 4 additions & 1 deletion client/components/registration/registration.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ const createTestProps = (props, configName = "default") => {
const config = getConfig(configName);
return {
language: "en",
defaultLanguage: "en",
orgSlug: configName,
orgName: config.name,
settings: config.settings,
Expand All @@ -54,7 +55,9 @@ const responseData = {
};

describe("<Registration /> rendering with placeholder translation tags", () => {
const props = createTestProps();
const props = createTestProps({
defaultLanguage: "en",
});
const wrapper = shallow(<Registration {...props} />, {
context: loadingContextValue,
});
Expand Down
51 changes: 34 additions & 17 deletions client/components/status/status.js
Original file line number Diff line number Diff line change
Expand Up @@ -338,7 +338,7 @@ export default class Status extends React.Component {
const auth_token = cookies.get(`${orgSlug}_auth_token`);
handleSession(orgSlug, auth_token, cookies);
const options = {radiusUsageSpinner: false};
let isPlanExhausted = false;

try {
const response = await axios({
method: "get",
Expand All @@ -348,45 +348,62 @@ export default class Status extends React.Component {
},
url,
});
options.userChecks = response.data.checks;
if (options.userChecks) {
options.userChecks.forEach((check) => {
if (check.value === String(check.result)) {
isPlanExhausted = true;
}
});

// Early return if no checks data
if (!response.data.checks || response.data.checks.length === 0) {
if (response.data.plan) {
options.userPlan = response.data.plan;
}
this.setState(options);
return;
}
Comment on lines +353 to 359
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the there are no user checks, then we need to set showRadiusUsage to false. Otherwise, we will keep showing an empty box on the status page.


// Handle checks data
options.userChecks = response.data.checks;
let isPlanExhausted = false;

options.userChecks.forEach((check) => {
if (check.value === String(check.result)) {
isPlanExhausted = true;
}
});

if (planExhausted !== isPlanExhausted) {
setPlanExhausted(isPlanExhausted);
if (isPlanExhausted) {
toast.info(t`PLAN_EXHAUSTED_TOAST`);
}
}
if (response.data.plan) {
options.userPlan = response.data.plan;
}

// Enable radius usage display if we have checks
if (!showRadiusUsage) {
options.showRadiusUsage = true;
}

// Handle plan data if present
if (response.data.plan) {
options.userPlan = response.data.plan;
}

Comment on lines +351 to +387
Copy link
Member

@pandafy pandafy Mar 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My earlier observation was incorrect. The return early does not handle all the scenarios.

I tested the following code locally. @Unnati-Gupta24, what do you think of these changes?

Suggested change
// Early return if no checks data
if (!response.data.checks || response.data.checks.length === 0) {
if (response.data.plan) {
options.userPlan = response.data.plan;
}
this.setState(options);
return;
}
// Handle checks data
options.userChecks = response.data.checks;
let isPlanExhausted = false;
options.userChecks.forEach((check) => {
if (check.value === String(check.result)) {
isPlanExhausted = true;
}
});
if (planExhausted !== isPlanExhausted) {
setPlanExhausted(isPlanExhausted);
if (isPlanExhausted) {
toast.info(t`PLAN_EXHAUSTED_TOAST`);
}
}
if (response.data.plan) {
options.userPlan = response.data.plan;
}
// Enable radius usage display if we have checks
if (!showRadiusUsage) {
options.showRadiusUsage = true;
}
// Handle plan data if present
if (response.data.plan) {
options.userPlan = response.data.plan;
}
// Handle plan data if present
if (response.data.plan) {
options.userPlan = response.data.plan;
}
options.showRadiusUsage = (response.data.checks && response.data.checks.length > 0)
// Handle checks data
if (options.showRadiusUsage) {
options.userChecks = response.data.checks;
let isPlanExhausted = false;
options.userChecks.forEach((check) => {
if (check.value === String(check.result)) {
isPlanExhausted = true;
}
});
if (planExhausted !== isPlanExhausted) {
setPlanExhausted(isPlanExhausted);
if (isPlanExhausted) {
toast.info(t`PLAN_EXHAUSTED_TOAST`);
}
}
}

this.setState(options);
} catch (error) {
// logout only if unauthorized or forbidden
// Early return for client-side errors
if (error.response) {
if (error.response.status === 401 || error.response.status === 403) {
logout(cookies, orgSlug);
toast.error(t`ERR_OCCUR`, {
onOpen: () => toast.dismiss(mainToastId),
});
} else if (
error.response.status >= 400 &&
error.response.status < 500
) {
// Do not retry for client side errors
return;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is good, thank you!

}

if (error.response.status >= 400 && error.response.status < 500) {
logError(error, t`ERR_OCCUR`);
this.setState({showRadiusUsage: false});
return;
}
}

logError(error, t`ERR_OCCUR`);
setTimeout(async () => {
this.getUserRadiusUsage();
Expand Down
81 changes: 77 additions & 4 deletions client/components/status/status.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ const getLinkText = (wrapper, selector) => {

const createTestProps = (props) => ({
language: "en",
defaultLanguage: "en",
orgSlug: "default",
orgName: "default name",
statusPage: defaultConfig.components.status_page,
Expand Down Expand Up @@ -94,9 +95,11 @@ const responseData = {
};

describe("<Status /> rendering with placeholder translation tags", () => {
const props = createTestProps();
props.statusPage.radius_usage_enabled = true;
it("should render translation placeholder correctly", () => {
const props = createTestProps({
defaultLanguage: "en", // Add the required defaultLanguage prop
});
props.statusPage.radius_usage_enabled = true;
const renderer = new ShallowRenderer();
const wrapper = renderer.render(<Status {...props} />);
expect(wrapper).toMatchSnapshot();
Expand All @@ -107,7 +110,9 @@ describe("<Status /> rendering", () => {
let props;

it("should render correctly", () => {
props = createTestProps();
props = createTestProps({
defaultLanguage: "en",
});
props.statusPage.radius_usage_enabled = true;
const renderer = new ShallowRenderer();
loadTranslation("en", "default");
Expand Down Expand Up @@ -1674,8 +1679,11 @@ describe("<Status /> interactions", () => {
it("test getUserRadiusUsage method", async () => {
jest.spyOn(toast, "error");
jest.spyOn(toast, "dismiss");

// Mock axios calls in sequence
axios
.mockImplementationOnce(() =>
// First call fails with 500
Promise.reject({
response: {
status: 500,
Expand All @@ -1684,10 +1692,12 @@ describe("<Status /> interactions", () => {
}),
)
.mockImplementationOnce(() =>
// Second call returns checks data
Promise.resolve({
status: 200,
statusText: "OK",
data: {
response: "radius_sessions_retrieved_successfully",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is this coming from?

checks: [
{
attribute: "Max-Daily-Session",
Expand All @@ -1702,10 +1712,12 @@ describe("<Status /> interactions", () => {
}),
)
.mockImplementationOnce(() =>
// Third call returns plan data
Promise.resolve({
status: 200,
statusText: "OK",
data: {
response: "radius_usage_retrieved_successfully",
plan: {
id: "d5bc4d5a-0a8c-4e94-8d52-4c54836bd013",
name: "Free",
Expand All @@ -1719,10 +1731,12 @@ describe("<Status /> interactions", () => {
}),
)
.mockImplementationOnce(() =>
// Fourth call returns both checks and plan
Promise.resolve({
status: 200,
statusText: "OK",
data: {
response: "radius_usage_retrieved_successfully",
checks: [
{
attribute: "Max-Daily-Session",
Expand All @@ -1745,32 +1759,43 @@ describe("<Status /> interactions", () => {
}),
)
.mockImplementationOnce(() =>
// Fifth call fails with 401
Promise.reject({
response: {
status: 401,
headers: {},
},
}),
);

props = createTestProps();
wrapper = shallow(<Status {...props} />, {
context: {setLoading: jest.fn()},
disableLifecycleMethods: true,
});
jest.spyOn(wrapper.instance(), "getUserRadiusUsage");

// Test first call - 500 error
wrapper.instance().getUserRadiusUsage();
await tick();
expect(wrapper.instance().state.radiusUsageSpinner).toBe(true);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know you haven't changed this line, but shouldn't this be false?

if (error.response.status >= 400 && error.response.status < 500) {
logError(error, t`ERR_OCCUR`);
this.setState({showRadiusUsage: false});
return;
}


// Test second call - get checks
wrapper.instance().getUserRadiusUsage();
await tick();
expect(wrapper.instance().state.userChecks.length).toBe(1);

// Test third call - get plan
wrapper.instance().getUserRadiusUsage();
await tick();
expect(wrapper.instance().state.userPlan.is_free).toBe(true);
Comment on lines 1788 to 1790
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this scenario, when the response didn't include checks, we should verify that showRadiusUsage was set to false.


// Test fourth call - plan exhausted
wrapper.instance().getUserRadiusUsage();
await tick();
expect(props.setPlanExhausted).toHaveBeenCalledTimes(1);
expect(props.setPlanExhausted).toHaveBeenCalledWith(true);

// Test fifth call - 401 unauthorized
wrapper.instance().getUserRadiusUsage();
await tick();
expect(toast.error.mock.calls.length).toBe(1);
Expand Down Expand Up @@ -1990,3 +2015,51 @@ describe("<Status /> interactions", () => {
});
});
});

describe("getUserRadiusUsage method", () => {
it("should update state with userChecks, userPlan and handle plan exhaustion", async () => {
// Simulate a response with checks that indicate plan exhaustion and a plan object
const testChecks = [{value: "1", result: "1"}];
const testPlan = {is_free: true};
axios.mockImplementationOnce(() =>
Promise.resolve({
status: 200,
data: {
checks: testChecks,
plan: testPlan,
},
}),
);
const props = createTestProps({defaultLanguage: "en"});
props.statusPage.radius_usage_enabled = true;
// Ensure flags for controlling view are set as needed (if applicable)
props.planExhausted = false;

const wrapper = shallow(<Status {...props} />, {
context: {setLoading: jest.fn()},
disableLifecycleMethods: true,
});

// Spy on setPlanExhausted and toast.info
const spySetPlanExhausted = jest.spyOn(props, "setPlanExhausted");
const spyToastInfo = jest.spyOn(toast, "info");

// Call the method and wait for asynchronous updates
await wrapper.instance().getUserRadiusUsage();
await tick();

// Validate that the state was updated correctly
expect(wrapper.instance().state.userChecks).toEqual(testChecks);
expect(wrapper.instance().state.userPlan).toEqual(testPlan);

// Since our check values indicate exhaustion, setPlanExhausted should be called with true
expect(spySetPlanExhausted).toHaveBeenCalledWith(true);
// Verify that a toast was shown for plan exhaustion
expect(spyToastInfo).toHaveBeenCalledWith(
expect.stringContaining("PLAN_EXHAUSTED_TOAST"),
);

// Also expect that radiusUsageSpinner is set to false (as initialized)
expect(wrapper.instance().state.radiusUsageSpinner).toBe(false);
});
});
Loading