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

Add login interface #13 #39

Merged
merged 12 commits into from
Aug 14, 2024
Merged

Add login interface #13 #39

merged 12 commits into from
Aug 14, 2024

Conversation

jacobtylerwalls
Copy link
Member

@jacobtylerwalls jacobtylerwalls commented Aug 5, 2024

Closes #13

Depends on archesproject/arches#11284 and archesproject/arches#11331

Styling will be finalized later.

Testing instructions

Visit the site (at root), ensure you don't have a csrf cookie (first-time visitor).

  • Login works in success path
  • Appropriate message shown in failure path
  • Logout works
  • Links to register, existing auth form work
  • Redirect from auth form doesn't go to login page
  • Refresh doesn't go to login page
  • Superuser can navigate directly to django admin

Copy link
Contributor

@chrabyrd chrabyrd left a comment

Choose a reason for hiding this comment

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

looks good! Just a few Qs 👍

tests/tests.py Outdated Show resolved Hide resolved
arches_lingo/templates/arches_lingo/root.htm Outdated Show resolved Hide resolved
arches_lingo/src/arches_lingo/api.ts Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this belong under components? IMO it's a top-level thing

Copy link
Member Author

Choose a reason for hiding this comment

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

What do you think about the App.vue component being the only top-level component outside components? As currently implemented, it uses <component> to switch between HomePage and LoginPage, but if you don't like that pattern, we can have the LoginPage be the top level component instead of App.vue.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah -- fun discussion abounds 😄 . This is an interesting topic for me.

As I understand it you're suggesting:

.
└── arches-lingo/
    └── arches_lingo/
        └── src/
            └── arches_lingo/
                ├── App.vue
                └── components/
                    └── login/
                        ├── LoginPage.vue
                        ├── LoginLinks.vue
                        └── LoginForm.vue

which is fine. The only problem I have with it is that it combines more-basic stuff with more-complex stuff. I guess unofficially I've been trying to steer us towards atomic design, and this violates that to some degree. Playing it out I could see:

.
└── arches-lingo/
    └── arches_lingo/
        └── src/
            └── arches_lingo/
                ├── App.vue
                └── components/
                    ├── login/
                    │   ├── LoginPage.vue
                    │   ├── LoginLinks.vue
                    │   └── LoginForm.vue
                    ├── foo/
                    │   ├── FooStuff.vue
                    │   └── ThingThatUsesFooStuff.vue
                    ├── bar/
                    │   ├── BarComponents.vue
                    │   ├── BarForm.vue
                    │   └── ThingThatUsesBarForm.vue
                    └── baz/
                        └── ThingThatUsesFooStuffAndBarForm.vue ?

where baz is a complex thing (eg page) that still lives in the components directory. Maybe it's just me being pedantic ( 😅 ) but a page shouldn't live among its componentry.

Are you open to something like views or pages?

Copy link
Member Author

Choose a reason for hiding this comment

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

Are you open to something like views or pages?

Definitely, I'm just flying a little blind as I don't know how many pages or views we're going to have. More than one?

Copy link
Member Author

Choose a reason for hiding this comment

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

Comment on lines +41 to +42
<HomePage v-if="user && user.username !== 'anonymous'" />
<LoginPage v-else />
Copy link
Member Author

Choose a reason for hiding this comment

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

This will change on the routing branch. For now this is just to get something functional to test with.

Copy link
Contributor

@chrabyrd chrabyrd left a comment

Choose a reason for hiding this comment

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

lgtm 👍

@jacobtylerwalls jacobtylerwalls merged commit 035f9e8 into main Aug 14, 2024
4 checks passed
@jacobtylerwalls jacobtylerwalls deleted the jtw/login-form branch August 14, 2024 22:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement token-based user authentication front end
2 participants