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

feat: autoregister commands & add enableAutoLogin setup #101

Merged
merged 5 commits into from Nov 25, 2020

Conversation

Mohammer5
Copy link
Contributor

@Mohammer5 Mohammer5 commented Nov 19, 2020

Fixes CLI-14

  • Autoregister all commands
  • Add enableAutoLogin setup helper
  • ~~Commit 33d7a6d (feat(login): add command to fill login form) is a bit questionable
    • REMOVED
    • Right now I'd prefer to not add a command for this at all
    • But I can see that some apps will have to fill in form data
    • We could either drop this command (by removing the commit) or keep it

@Mohammer5 Mohammer5 marked this pull request as ready for review November 19, 2020 11:42
@Mohammer5 Mohammer5 requested a review from varl November 19, 2020 11:43
@Mohammer5 Mohammer5 changed the title refactor: autoregister commands feat: autoregister commands & add enableAutoLogin setup Nov 19, 2020
Copy link
Contributor

@HendrikThePendric HendrikThePendric left a comment

Choose a reason for hiding this comment

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

This will be very handy, great work! I've left a few comments in the code, most relate to the same thing so I'll also try to explain my general ideas below:

  • In my opinion, if possible, we should only have one login command and ideally this should be doing what is now loginBasicAuth, since that is independent of the app-platform login modal.
  • And besides that login command we should have the enableAutoLogin setup, which calls cy.login() in the before hook and in the beforeEach it preserves the cookie and localStorage item. Probably I would prefer a slightly different name, i.e. enablePersistedSession or sth.

packages/cypress-commands/src/setups/enableAutoLogin.js Outdated Show resolved Hide resolved
packages/cypress-commands/src/index.js Outdated Show resolved Hide resolved
packages/cypress-commands/src/commands/login.js Outdated Show resolved Hide resolved
packages/cypress-commands/src/commands/login.js Outdated Show resolved Hide resolved
@Mohammer5
Copy link
Contributor Author

@HendrikThePendric I've updated the login command, the setup file and the deprecation warning

Copy link
Contributor

@HendrikThePendric HendrikThePendric left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the points we discussed. Great work!

@Mohammer5 Mohammer5 merged commit bfd79c1 into master Nov 25, 2020
@Mohammer5 Mohammer5 deleted the CLI-14_Autoregister_commands branch November 25, 2020 13:00
@dhis2-bot
Copy link
Contributor

🎉 This PR is included in version 3.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

None yet

5 participants