fix(appRoles): don't allowed to upload duplicate app roles #877

tfjanjua · 2024-07-26T14:07:48Z

Description

eliminated duplicate app role values from the APIs: /api/apps/appreleaseprocess/{appId}/role & /api/apps/AppChange/{appId}/role/activeapp

Why

An app should have unique roles, duplicate roles are causing issue while assigning app roles to any company user.

Issue

Ref: #868

Checklist

Please delete options that are not relevant.

I have followed the contributing guidelines
I have performed a self-review of my own code
I have successfully tested my changes locally
I have added tests that prove my changes work
I have checked that new and existing tests pass locally with my changes
I have commented my code, particularly in hard-to-understand areas

- eliminate duplicate role values from api: /api/apps/appreleaseprocess/{appId}/role & /api/apps/AppChange/{appId}/role/activeapp - added new api to batch delete recently uploaded app roles: /api/apps/appreleaseprocess/{appId}/roles:batchDelete [Ref: eclipse-tractusx#868](eclipse-tractusx#868)

src/portalbackend/PortalBackend.DBAccess/Repositories/UserRolesRepository.cs

tests/marketplace/Apps.Service.Tests/BusinessLogic/AppChangeBusinessLogicTest.cs

tests/marketplace/Apps.Service.Tests/BusinessLogic/AppReleaseBusinessLogicTest.cs

tests/marketplace/Apps.Service.Tests/BusinessLogic/AppChangeBusinessLogicTest.cs

src/portalbackend/PortalBackend.DBAccess/Repositories/UserRolesRepository.cs

Unreadable error message is been respnded by the API, when admin or manager was trying to assign duplicate app roles to a user. before fix: roles System.Linq.Enumerable\u002BSelectEnumerableIterator\u00602[Org.Eclipse.TractusX.Portal.Backend.PortalBackend.DBAccess.Models.UserRoleModificationData,System.String] are ambigous after fix: roles [Manager, 779c63ba-025c-4d5e-a4e8-cd3c827eb49c] are ambigous

evegufy

Hi @tfjanjua thank you for the contribution! could you please resolve the remaining codeQL findings "Useless assignment to local variable"?

tfjanjua · 2024-08-05T11:19:36Z

Hi @tfjanjua thank you for the contribution! could you please resolve the remaining codeQL findings "Useless assignment to local variable"?

Hi @evegufy , its resolved

evegufy · 2024-08-05T12:16:31Z

Hi @ntruchsess @Phil91 could you please review?

src/marketplace/Apps.Service/Extensions/AppExtensions.cs

src/portalbackend/PortalBackend.DBAccess/Repositories/UserRolesRepository.cs

src/marketplace/Apps.Service/BusinessLogic/AppReleaseBusinessLogic.cs

evegufy · 2024-08-06T09:31:29Z

merge_postponed as a decision about the endpoint semantics need be made, @MaximilianHauer could you please provide your feedback?

- Removed Distinct from AppExtension (Zip) - Removed checks from Repository and moved it to BusinessLogic - Updated local veriables

tfjanjua · 2024-08-12T09:29:59Z

merge_postponed as a decision about the endpoint semantics need be made, @MaximilianHauer could you please provide your feedback?

Hi @evegufy , do you think we can merge this PR (if all looks good) and then frontend will take their part as per their capacity? because it will not break any frontend functionality but it would be nice if frontend would also work on it and resolve their part (Please check the Proposed Solution heading: #868)
I ask this because we need bug fix from this PR (stopping user from uploading duplicate app user roles) and implementation of new API (to bulk delete recently added App User Roles) is good to have but not urgent.

I can also separate bug (stopping user from uploading duplicate app user roles bug fix) and new API (to bulk delete recently added Roles) in separate PRs so, we could have bug fix quickly available.

Just FYI: we already have frontend capacity available to work on frontend part related to resolving the bug (stopping user from uploading duplicate app user roles bug fix) and implementing the new API (to bulk delete recently added Roles)
eclipse-tractusx/portal-frontend#969

cc: @ybidois | @MaximilianHauer | @ntruchsess | @Phil91

evegufy · 2024-08-12T13:34:28Z

merge_postponed as a decision about the endpoint semantics need be made, @MaximilianHauer could you please provide your feedback?

Hi @evegufy , do you think we can merge this PR (if all looks good) and then frontend will take their part as per their capacity? because it will not break any frontend functionality but it would be nice if frontend would also work on it and resolve their part (Please check the Proposed Solution heading: #868) I ask this because we need bug fix from this PR (stopping user from uploading duplicate app user roles) and implementation of new API (to bulk delete recently added App User Roles) is good to have but not urgent.

I can also separate bug (stopping user from uploading duplicate app user roles bug fix) and new API (to bulk delete recently added Roles) in separate PRs so, we could have bug fix quickly available.

Just FYI: we already have frontend capacity available to work on frontend part related to resolving the bug (stopping user from uploading duplicate app user roles bug fix) and implementing the new API (to bulk delete recently added Roles) eclipse-tractusx/portal-frontend#969

cc: @ybidois | @MaximilianHauer | @ntruchsess | @Phil91

Hi @tfjanjua,

afaik, @ntruchsess @MaximilianHauer were discussing about the implementation approach with the batch delete...
So yes, I think separating the bug and the new API, would help to get the bugfix in sooner.
Before you do that, please wait for feedback from @ntruchsess and @MaximilianHauer.

src/marketplace/Apps.Service/Extensions/AppExtensions.cs

…ecenlty added App User Roles

tfjanjua · 2024-08-13T10:28:08Z

Hi @evegufy, I discussed with @ntruchsess and he explained the business behind this App User Roles and as per his explanation, it makes sense to replace all the App User Roles when user re-upload them and keep only the new ones instead of all roles (like previously uploaded and recently added from Add Roles option of any Active app) and just ignore the duplicate roles.

So, I have removed the implementation of new API (to delete recently uploaded App User Roles) from this PR and now this PR only has a fix to stop users to upload duplicate roles and we need this fix for our release because if user uploads duplicate roles from any screen (App Onboarding->Technical Integration screen OR Load Additional Roles) so, later ending up encountering 409 conflict errors while assigning those duplicated App User Roles to a Company Users.

Later (as we can see @Phil91 has added this PR in release 24.12) we can implement/change the existing API of Upload App User Roles as per @ntruchsess 's explanation but this fix will stop users to upload duplicate roles till r24.12.

So, please let me know if this PR can be merge OR should I close this PR and create another one? that new PR will also have 'Stop user to upload duplicate roles' fix.

cc: @ybidois | @MaximilianHauer

evegufy · 2024-08-13T17:14:29Z

Hi @evegufy, I discussed with @ntruchsess and he explained the business behind this App User Roles and as per his explanation, it makes sense to replace all the App User Roles when user re-upload them and keep only the new ones instead of all roles (like previously uploaded and recently added from Add Roles option of any Active app) and just ignore the duplicate roles.

So, I have removed the implementation of new API (to delete recently uploaded App User Roles) from this PR and now this PR only has a fix to stop users to upload duplicate roles and we need this fix for our release because if user uploads duplicate roles from any screen (App Onboarding->Technical Integration screen OR Load Additional Roles) so, later ending up encountering 409 conflict errors while assigning those duplicated App User Roles to a Company Users.

Later (as we can see @Phil91 has added this PR in release 24.12) we can implement/change the existing API of Upload App User Roles as per @ntruchsess 's explanation but this fix will stop users to upload duplicate roles till r24.12.

So, please let me know if this PR can be merge OR should I close this PR and create another one? that new PR will also have 'Stop user to upload duplicate roles' fix.

cc: @ybidois | @MaximilianHauer

@tfjanjua good, if it only contains the fix to stop users from uploading duplicate roles, I'll change the milestone to 2.2.0 and the base to the according release branch.
could you please still update the PR description so that it reflects only the fix? Please also update the PR title to a more conventional commit aligned message, like fix(appRoles): don't allowed to upload duplicate app roles
@ntruchsess could you please also confirm that this change is fine for you by approving?

src/marketplace/Apps.Service/Extensions/AppExtensions.cs

sonarqubecloud · 2024-08-14T07:56:07Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

code-ql findings are resoved

tfjanjua self-assigned this Jul 26, 2024

tfjanjua requested a review from evegufy July 26, 2024 14:08

tfjanjua added bug Something isn't working Improvement labels Jul 26, 2024

github-advanced-security bot found potential problems Jul 26, 2024

View reviewed changes

fix: Code scanning / CodeQL

a86789d

github-advanced-security bot found potential problems Jul 29, 2024

View reviewed changes

src/portalbackend/PortalBackend.DBAccess/Repositories/UserRolesRepository.cs Fixed Show resolved Hide resolved

src/portalbackend/PortalBackend.DBAccess/Repositories/UserRolesRepository.cs Fixed Show resolved Hide resolved

tfjanjua requested review from ntruchsess and Phil91 July 29, 2024 18:38

evegufy previously requested changes Aug 5, 2024

View reviewed changes

fix: Code scanning / CodeQL

e371eb5

tfjanjua requested a review from evegufy August 5, 2024 11:19

ntruchsess requested changes Aug 5, 2024

View reviewed changes

ntruchsess mentioned this pull request Aug 5, 2024

App Release | App manager is allowed to upload duplicate roles #868

Closed

ntruchsess linked an issue Aug 5, 2024 that may be closed by this pull request

App Release | App manager is allowed to upload duplicate roles #868

Closed

typecastcloud requested a review from dhiren-singh-007 August 6, 2024 07:37

evegufy added the merge postponed the merge of this PR shall be postponed until all prerequisites are fulfilled label Aug 6, 2024

evegufy assigned MaximilianHauer Aug 6, 2024

tfjanjua added 2 commits August 7, 2024 09:14

fix(reviewChanges): PR review changes

2f11b2a

- Removed Distinct from AppExtension (Zip) - Removed checks from Repository and moved it to BusinessLogic - Updated local veriables

fix(duplicatedLines): removed/optimize duplicated lines of code

c0b60e7

tfjanjua requested a review from ntruchsess August 7, 2024 10:54

Phil91 added this to the Release 24.12 milestone Aug 12, 2024

ntruchsess requested changes Aug 13, 2024

View reviewed changes

src/marketplace/Apps.Service/Extensions/AppExtensions.cs Show resolved Hide resolved

src/marketplace/Apps.Service/Extensions/AppExtensions.cs Outdated Show resolved Hide resolved

src/marketplace/Apps.Service/Extensions/AppExtensions.cs Outdated Show resolved Hide resolved

fix(reviewChanges): PR review changes and removed new API to delete r…

3e5db04

…ecenlty added App User Roles

tfjanjua requested a review from ntruchsess August 13, 2024 10:28

evegufy changed the base branch from main to release/v2.2.0-RC2 August 13, 2024 17:14

evegufy modified the milestones: Release 24.12, Release 2.2.0 Aug 13, 2024

evegufy removed merge postponed the merge of this PR shall be postponed until all prerequisites are fulfilled Improvement labels Aug 13, 2024

evegufy force-pushed the release/v2.2.0-RC2 branch from 801050d to 6ca675f Compare August 14, 2024 07:50

cosmetics

ce17471

ntruchsess approved these changes Aug 14, 2024

View reviewed changes

src/marketplace/Apps.Service/Extensions/AppExtensions.cs Show resolved Hide resolved

tfjanjua changed the title ~~fix(appRoles): User is allowed to upload duplicate app roles~~ fix(appRoles): don't allowed to upload duplicate app roles Aug 14, 2024

evegufy approved these changes Aug 14, 2024

View reviewed changes

evegufy merged commit 11390e5 into eclipse-tractusx:release/v2.2.0-RC2 Aug 14, 2024
13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(appRoles): don't allowed to upload duplicate app roles #877

fix(appRoles): don't allowed to upload duplicate app roles #877

tfjanjua commented Jul 26, 2024 •

edited by ntruchsess

Loading

evegufy left a comment •

edited

Loading

tfjanjua commented Aug 5, 2024

evegufy commented Aug 5, 2024

evegufy commented Aug 6, 2024

tfjanjua commented Aug 12, 2024

evegufy commented Aug 12, 2024 •

edited

Loading

tfjanjua commented Aug 13, 2024 •

edited

Loading

evegufy commented Aug 13, 2024

sonarqubecloud bot commented Aug 14, 2024

fix(appRoles): don't allowed to upload duplicate app roles #877

fix(appRoles): don't allowed to upload duplicate app roles #877

Conversation

tfjanjua commented Jul 26, 2024 • edited by ntruchsess Loading

Description

Why

Issue

Checklist

evegufy left a comment • edited Loading

Choose a reason for hiding this comment

tfjanjua commented Aug 5, 2024

evegufy commented Aug 5, 2024

evegufy commented Aug 6, 2024

tfjanjua commented Aug 12, 2024

evegufy commented Aug 12, 2024 • edited Loading

tfjanjua commented Aug 13, 2024 • edited Loading

evegufy commented Aug 13, 2024

sonarqubecloud bot commented Aug 14, 2024

Quality Gate passed

tfjanjua commented Jul 26, 2024 •

edited by ntruchsess

Loading

evegufy left a comment •

edited

Loading

evegufy commented Aug 12, 2024 •

edited

Loading

tfjanjua commented Aug 13, 2024 •

edited

Loading