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

Improve instructions for deprecated "Web Apps Permissions" feature #35245

Merged
merged 11 commits into from
Oct 24, 2024

Conversation

Jtang-1
Copy link
Contributor

@Jtang-1 Jtang-1 commented Oct 22, 2024

Product Description

The goal of these changes are to 1. discourage using both the Roles & Permissions page and the deprecated Manage Web Apps Permissions page simultaneously and 2. provides users with a way to opt out of the deprecated feature.

Summary of Changes:

  • Updated Wording for Configuration Options:.
    • The previous option, "Allow all mobile workers to see all Web Apps applications," is now aligned with using only the configuration on the Roles & Permissions page.
  • Opt-out Mechanism for Deprecated Page:
    • When "Use Roles & Permissions to manage Web Apps access" is selected, a checkbox appears, allowing users to remove the domain from the deprecated feature flag (FF). Upon saving, this action removes the Manage Web Apps Permissions page from the domain.
  • Transition Guidance:
    • Added instructions to assist users in migrating from the deprecated page to the updated configuration.

Before:
Screen Shot 2024-10-22 at 11 32 05 AM

After:
Screen Shot 2024-10-24 at 11 09 35 AM

  • If the web_apps_permissions_via_groups FF is enabled, and the option "Manage Web Apps access for each mobile worker based on their group" is selected, a warning is displayed when configuring Web Apps access on the Roles & Permissions page.

Screen Shot 2024-10-24 at 11 10 00 AM

Technical Summary

USH-4996

Feature Flag

Relates to web_apps_permissions_via_groups FFthat enabled this deprecated feature for certain domains

Safety Assurance

Safety story

locally tested. The change is limited to altering text or display logic for showing text.

Automated test coverage

no automated test

QA Plan

no QA

Rollback instructions

  • This PR can be reverted after deploy with no further considerations

Labels & Review

  • Risk label is set correctly
  • The set of people pinged as reviewers is appropriate for the level of risk of the change

@dimagimon dimagimon added the Risk: Medium Change affects files that have been flagged as medium risk. label Oct 22, 2024
@Jtang-1 Jtang-1 marked this pull request as ready for review October 22, 2024 18:59
Copy link
Contributor

@orangejenny orangejenny left a comment

Choose a reason for hiding this comment

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

This is so much more informative.

access.sqlappgroup_set.all().delete()
access.sqlappgroup_set.set([
SQLAppGroup(app_id=app_group['app_id'], group_id=app_group.get('group_id'))
for app_group in body['app_groups']
], bulk=False)
access.save()
if disable_ff and not access.restrict:
toggles.WEB_APPS_PERMISSIONS_VIA_GROUPS.set(self.domain, False, namespace=toggles.NAMESPACE_DOMAIN)
Copy link
Contributor

Choose a reason for hiding this comment

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

hooray

Copy link
Contributor

Choose a reason for hiding this comment

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

love to see it

corehq/apps/cloudcare/templates/cloudcare/config.html Outdated Show resolved Hide resolved
corehq/apps/cloudcare/templates/cloudcare/config.html Outdated Show resolved Hide resolved
corehq/apps/cloudcare/templates/cloudcare/config.html Outdated Show resolved Hide resolved
</li>
<li>
<strong>(Optional) Deactivate the deprecated feature</strong><br/>
Although not required for the new setup to work, you can disable the deprecated feature for cleaner management.
Copy link
Contributor

Choose a reason for hiding this comment

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

How about something like, "To prevent this feature being accidentally re-enabled in the future, disable it altogether."?

Copy link
Contributor Author

@Jtang-1 Jtang-1 Oct 24, 2024

Choose a reason for hiding this comment

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

e60bd9a

I reworded your suggestion a little to " Disable this feature to avoid accidentally re-enabling it in the future." , lmk if you think otherwise

corehq/apps/cloudcare/templates/cloudcare/config.html Outdated Show resolved Hide resolved
access.sqlappgroup_set.all().delete()
access.sqlappgroup_set.set([
SQLAppGroup(app_id=app_group['app_id'], group_id=app_group.get('group_id'))
for app_group in body['app_groups']
], bulk=False)
access.save()
if disable_ff and not access.restrict:
toggles.WEB_APPS_PERMISSIONS_VIA_GROUPS.set(self.domain, False, namespace=toggles.NAMESPACE_DOMAIN)
Copy link
Contributor

Choose a reason for hiding this comment

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

love to see it

if disable_ff and not access.restrict:
toggles.WEB_APPS_PERMISSIONS_VIA_GROUPS.set(self.domain, False, namespace=toggles.NAMESPACE_DOMAIN)
# This view is not accessible after the FF is disabled
redirect_url = reverse('users_default', args=[self.domain])
Copy link
Contributor

Choose a reason for hiding this comment

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

should this direct them to the roles and permissions page instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes that would be better 1eed3e8

@Jtang-1 Jtang-1 merged commit ef4e1cc into master Oct 24, 2024
13 checks passed
@Jtang-1 Jtang-1 deleted the jt/improve_web_apps_permission_ga_user branch October 24, 2024 18:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Risk: Medium Change affects files that have been flagged as medium risk.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants