Skip to content

Respect doNotPromptForParty profile preference #1204

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

Merged
merged 10 commits into from
Jun 8, 2023

Conversation

bjosttveit
Copy link
Member

@bjosttveit bjosttveit commented May 26, 2023

Description

Respect the profile preference that implies that you should always be presented with party selection on new instantiation unless you specifically ask not to have this happen.

What happenes now?

  1. doNotPromptForParty is by default unchecked in the altinn profile
  2. If you have more than one valid party that can instantiate, it will always show party selection
  3. If you have exactly one valid party it will not do this

To make this possible I had to always pull the list of valid parties and store in state.party.parties, before, it would only get this list when entering party selection, and would just put the current party in otherwise. This change will possibly fix Altinn/app-lib-dotnet#244 as well.

I manually updated the profile preferences for all of the test users in tt02 that we use in cypress to have doNotPromptForParty=true, that way all the existing tests work without modification. I have also updated this in app-localtest: Altinn/app-localtest#42

Related Issue(s)

Verification/QA

  • Manual functionality testing
    • I have tested these changes manually
    • Creator of the original issue (or service owner) has been contacted for manual testing (or will be contacted when released in alpha)
    • No testing done/necessary
  • Automated tests
    • Unit test(s) have been added/updated
    • Cypress E2E test(s) have been added/updated
    • No automatic tests are needed here (no functional changes/additions)
    • I want someone to help me make some tests
  • UU/WCAG (follow these guidelines until we have our own)
    • I have tested with a screen reader/keyboard navigation/automated wcag validator
    • No testing done/necessary (no DOM/visual changes)
    • I want someone to help me perform accessibility testing
  • User documentation @ altinn-studio-docs
    • Has been added/updated
    • No functionality has been changed/added, so no documentation is needed
    • I will do that later/have created an issue
  • Changes/additions to component properties
    • Changes are reflected in both src/layout/layout.d.ts and layout.schema.v1.json, and these are all backwards-compatible
    • No changes made
  • Support in Altinn Studio
    • Issue(s) created for support in Studio
    • This change/feature does not require any changes to Altinn Studio
  • Sprint board
    • The original issue (or this PR itself) has been added to the Team Apps project and to the current sprint board
    • I don't have permissions to do that, please help me out
  • Labels
    • I have added a kind/* label to this PR for proper release notes grouping
    • I don't have permissions to add labels, please help me out

@bjosttveit bjosttveit added the kind/bug Something isn't working label May 26, 2023
Copy link
Contributor

@Magnusrm Magnusrm left a comment

Choose a reason for hiding this comment

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

Amazing work! 🚀

@olemartinorg
Copy link
Contributor

Isn't this, in a way, a breaking change? Behavior seems to change from how it was before, at least. I know we'll get complaints if we just roll this out in a patch release, even if it just a bugfix for a property we didn't properly respect before. How do we manage this to properly notify everyone? Calling on @RonnyB71. I know at least some external cypress tests will fail if this selector suddenly interferes - we're not the only ones running e2e-tests on apps.

@bjosttveit
Copy link
Member Author

bjosttveit commented Jun 2, 2023

Okey so what should be done about this?

  1. I have not added a way for specific apps to ignore this setting, but you want that to be possible @RonnyB71 ? Where should this configuration live? applicationMetadata? Should it be opt-in or opt-out?
  2. Having some text explaining why the user is seeing the party selection every time and informing them how to change their preference is a good idea. Should we add that too @RonnyB71 ?
  3. As for the potential failing external cypress tests @olemartinorg , what do you suggest we do about that? If we make the setting opt-in, it will essentially not exist since I dont see why app developers will bother opting in to something like this 🤷 Should this be opt-in before a new major version is released, and then on by default?

Also, if this issue need some more time, should we fix the instance sender PDF bug in app-lib for now? Altinn/app-lib-dotnet#245

@bjosttveit bjosttveit marked this pull request as draft June 2, 2023 13:19
@olemartinorg
Copy link
Contributor

Should this be opt-in before a new major version is released, and then on by default?

That sounds like a good approach to me! 🤔

Another way could be to keep the implementation as it is, but make it a feature toggle in the app-frontend code. I'm doing that for #1175 in this file here, and then just using an if/else everywhere in the code so that I can toggle the new/old functionality. That way, we can remove the constant in v4 and roll it out then - in essence making breaking changes now that are deactivated until everyone are ready to start v4 (and avoiding having to maintain and merge back and forth between a main and v4 branch that are guaranteed to get out-of-sync quickly when we start making large sweeping changes).

@bjosttveit bjosttveit marked this pull request as ready for review June 7, 2023 13:24
@bjosttveit bjosttveit requested review from olemartinorg, Magnusrm and lassopicasso and removed request for olemartinorg, framitdavid and lassopicasso June 7, 2023 13:53
@bjosttveit
Copy link
Member Author

@olemartinorg let me know if you think the feature toggle strategy is fine

Copy link
Contributor

@olemartinorg olemartinorg left a comment

Choose a reason for hiding this comment

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

Looks very good! 🥳 I absolutely agree with the feature toggle functionality. In time I guess we should also make it controllable using a cookie (read on app load, set for each app using the /{org}/{app} path), and extend the developer tools to let you toggle on/off features in preview for the next major version (which should reload the page using that feature).

@sonarqubecloud
Copy link

sonarqubecloud bot commented Jun 8, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

28.2% 28.2% Coverage
0.0% 0.0% Duplication

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Something isn't working
Projects
None yet
3 participants