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

Fix dagl and regna bug in policy editor #12247

Merged
13 commits merged into from
Feb 14, 2024

Conversation

ghost
Copy link

@ghost ghost commented Feb 5, 2024

Description

  • Fixing the bug where both the code for dagl and regna was shown as well as the title for them.

Related Issue(s)

Verification

  • Your code builds clean without any errors or warnings
  • Manual testing done (required)
  • Relevant automated test added (if you find this hard, leave it and we'll help out)

@ghost ghost linked an issue Feb 5, 2024 that may be closed by this pull request
@github-actions github-actions bot added the solution/studio/designer Issues related to the Altinn Studio Designer solution. label Feb 5, 2024
Copy link

codecov bot commented Feb 5, 2024

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (60280e5) 86.41% compared to head (617775c) 86.42%.

Files Patch % Lines
...ents/ExpandablePolicyCard/ExpandablePolicyCard.tsx 75.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main   #12247   +/-   ##
=======================================
  Coverage   86.41%   86.42%           
=======================================
  Files        1198     1198           
  Lines       18066    18075    +9     
  Branches     2292     2294    +2     
=======================================
+ Hits        15612    15621    +9     
  Misses       2168     2168           
  Partials      286      286           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@framitdavid framitdavid left a comment

Choose a reason for hiding this comment

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

Great work! 🚀 Just some comments to consider. 😊

frontend/packages/policy-editor/src/utils/index.ts Outdated Show resolved Hide resolved
frontend/packages/policy-editor/src/utils/index.ts Outdated Show resolved Hide resolved
frontend/packages/policy-editor/src/utils/index.ts Outdated Show resolved Hide resolved
Copy link
Member

@nkylstad nkylstad left a comment

Choose a reason for hiding this comment

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

I see that these changes may help avoid that we display existing roles in the list of available roles to add.
However, they don't seem to solve the problem described in the linked issue, which is that apps with rolecodes that are in uppercase, have the roles displayed with the rolecode rather than the display text for the role 🤔

@ghost
Copy link
Author

ghost commented Feb 9, 2024

I see that these changes may help avoid that we display existing roles in the list of available roles to add. However, they don't seem to solve the problem described in the linked issue, which is that apps with rolecodes that are in uppercase, have the roles displayed with the rolecode rather than the display text for the role 🤔

@nkylstad, That is true, but I'm unsure if that is the case for any roles at the moment? 🤔
The reason it looked like it before this PR was that the DAGL and REGNA roles were added by default when creating a new app, and then the list was merged with the list of roles options. When looking through the list locally now, I cannot see any roles displayed as you mention.
It might be that I misunderstand this, but I believe it should work as expected now 😄

@ghost ghost requested review from nkylstad and framitdavid February 9, 2024 12:38
@nkylstad
Copy link
Member

nkylstad commented Feb 12, 2024

@nkylstad, That is true, but I'm unsure if that is the case for any roles at the moment? 🤔

The screenshot in the issue is from a real case in production where a user has noted that they used to see the role name, but now they just see the role code. F.ex. rule 2 in https://altinn.studio/editor/skd/rf-1551/overview has this issue.

I assume the problem is that some of the roles in that particular policy have uppercase rolecodes (which are allowed), but when we get the role display name we don't handle this.

@ghost
Copy link
Author

ghost commented Feb 12, 2024

@nkylstad, That is true, but I'm unsure if that is the case for any roles at the moment? 🤔

The screenshot in the issue is from a real case in production where a user has noted that they used to see the role name, but now they just see the role code. F.ex. rule 2 in https://altinn.studio/editor/skd/rf-1551/overview has this issue.

I assume the problem is that some of the roles in that particular policy have uppercase rolecodes (which are allowed), but when we get the role display name we don't handle this.

Se der ja! Jeg skal få sett på det 😄

@ghost ghost marked this pull request as draft February 12, 2024 20:05
@ghost ghost marked this pull request as ready for review February 13, 2024 20:25
@ghost
Copy link
Author

ghost commented Feb 13, 2024

@nkylstad, da skal problemet være løst og fikset 😄
image

Copy link
Member

@nkylstad nkylstad left a comment

Choose a reason for hiding this comment

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

Nice! 🥇

@standeren standeren self-assigned this Feb 14, 2024
WilliamThorenfeldt and others added 2 commits February 14, 2024 19:18
@ghost ghost merged commit 3e13953 into main Feb 14, 2024
10 checks passed
@ghost ghost deleted the 12080-remove-dagl-and-regna-bug-from-policy-editor branch February 14, 2024 19:25
@standeren standeren removed their assignment Feb 15, 2024
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
solution/studio/designer Issues related to the Altinn Studio Designer solution.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Policy editor should support displaying roles with upper and lower case rolecodes
4 participants