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

ConditionsModal: Fix selection of multiple items in different categories #14784

Merged
merged 1 commit into from Nov 4, 2022

Conversation

gu-stav
Copy link
Contributor

@gu-stav gu-stav commented Nov 4, 2022

What does it do?

Fixes the selection of multiple conditions across different categories.

Why is it needed?

Fixes the attached issue. The actual change is quite small, but I had to write a couple tests to understand the data structure 🤯 .

Instead of using one category only

Before

const [[, values]] = arrayOfOptionsGroupedByCategory;
  const formattedValues = values.reduce(
    (acc, curr) => ({ [curr.id]: val.includes(curr.id), ...acc }),
    {}
  );
);

we need to iterate over all categories.

After

arrayOfOptionsGroupedByCategory
  .map(([, values]) => values)
  .flat()
  .reduce((acc, curr) => ({ [curr.id]: changedValues.includes(curr.id), ...acc }), {});

How to test it?

  1. Create a new condition:
// examples/getstarted/src/index.js

'use strict';

module.exports = {
  // ...

  async bootstrap({ strapi }) {
    await strapi.admin.services.permission.conditionProvider.register({
      displayName: 'Billing amount under 10K',
      name: 'billing-amount-under-10k',
      plugin: 'admin',
      category: 'something',
      handler: { amount: { $lt: 10000 } },
    });
  },
  
  // ...
};
  1. Start Strapi using the EE edition
  2. Navigate to http://localhost:4000/admin/settings/roles/2
  3. Verify you can select multiple values across both categories

Related issue(s)/PR(s)

@gu-stav gu-stav added source: core:admin Source is core/admin package pr: fix This PR is fixing a bug labels Nov 4, 2022
@gu-stav gu-stav added this to the 4.5.0 milestone Nov 4, 2022
@codecov
Copy link

codecov bot commented Nov 4, 2022

Codecov Report

Base: 58.72% // Head: 58.76% // Increases project coverage by +0.03% 🎉

Coverage data is based on head (2d849bb) compared to base (933f646).
Patch coverage: 89.47% of modified lines in pull request are covered.

❗ Current head 2d849bb differs from pull request most recent head 5548344. Consider uploading reports for the commit 5548344 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #14784      +/-   ##
==========================================
+ Coverage   58.72%   58.76%   +0.03%     
==========================================
  Files        1339     1340       +1     
  Lines       32490    32491       +1     
  Branches     6143     6143              
==========================================
+ Hits        19079    19092      +13     
+ Misses      11523    11511      -12     
  Partials     1888     1888              
Flag Coverage Δ
front 63.25% <89.47%> (+0.05%) ⬆️
unit 48.82% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...Page/components/ConditionsModal/ActionRow/index.js 60.00% <33.33%> (+17.14%) ⬆️
...ponents/ConditionsModal/ActionRow/utils/options.js 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@derrickmehaffy
Copy link
Member

Ty for the quick fix!

Copy link
Member

@joshuaellis joshuaellis left a comment

Choose a reason for hiding this comment

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

Nice move to move the functions out of the component for testing ⭐

@gu-stav gu-stav merged commit c1de0e1 into main Nov 4, 2022
@gu-stav gu-stav deleted the fix/settings-conditions-category branch November 4, 2022 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: fix This PR is fixing a bug source: core:admin Source is core/admin package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RBAC - declaring new conditions with category breaks the admin ui
3 participants