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.

Sign up for GitHub

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

Jump to bottom

[NEW] store checked-in cookie and show checked-in alert #278

Open
wants to merge 11 commits into
base: dev
Choose a base branch
from
Open

[NEW] store checked-in cookie and show checked-in alert #278

wants to merge 11 commits into from

Conversation

maxones25
Copy link
Collaborator

@maxones25 maxones25 commented Oct 28, 2021 •
edited by Gabril-E
Loading

Related Issue

Proposed Changes

Additional Info

Checklist

  • Is the code formatted correctly?
  • Unnecessary comments removed?
  • Printout statements removed?
  • If you made backend changes: Did all tests pass? Do you need to adjust some tests or write new ones?
  • If you made frontend changes: Did you test the UI on different devices/ browsers (Firefox, Chrome, Safari, different smartphone sizes)
  • Is your code easy to understand or do you need to insert some comments?
  • Explain the issue for the reviewer and your steps to solve the issue => makes it a lot more easy for the reviewer
  • Label your pull request (frontend/ backend/ testing/ styling/ sql/...)
  • Is the open source message included at the very beginning of every source code file?
  • Are all imports organized, i.e. are the imports up to date and have all package imports been removed?

@maxones25
Copy link
Collaborator Author

image
image

@Vicctic Vicctic linked an issue Nov 11, 2021 that may be closed by this pull request
Store a check-in in a cookie so that a check-out can happen from any page #180
Open
@Gabril-E Gabril-E self-requested a review November 25, 2021 10:30
@Gabril-E
Copy link
Collaborator

Gabril-E commented Jan 2, 2022

I did some testing and everything seems fine. While reviewing your code i got stuck on the CookieManager class. Not sure if i understand what you are doing there, either you lead me through this or someone with experience in Cookie managing with javax should review this file.

oliverhummel
oliverhummel reviewed Jan 12, 2022
View reviewed changes
src/main/resources/static/general.css
@@ -0,0 +1,40 @@
.danger-color {
Copy link
Contributor

Choose a reason for hiding this comment

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

Am I right that this file is only used for the "still checked in message"? Then pls give it a more descriptive name such as checked-in-alert.css or so.

oliverhummel
oliverhummel reviewed Jan 12, 2022
View reviewed changes
src/main/resources/static/main.js
@@ -0,0 +1,7 @@
const checkInAlertForm = document.getElementById('check-in-alert-form')
Copy link
Contributor

Choose a reason for hiding this comment

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

I would also prefer a more descriptive name here.

oliverhummel
oliverhummel reviewed Jan 12, 2022
View reviewed changes
src/main/resources/static/general.css
flex-direction: column;
border: 2px solid #22376F;
border-radius: 3px;
margin: 5px;
Copy link
Contributor

Choose a reason for hiding this comment

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

May I suggest a somewhat larger margin below the box? Currently it is somewhat close to the regular text on the page. (not sure this is the right margin attribute though)

@oliverhummel
Update layout.html
6672336 
Added a period at the end of the sentence.
@oliverhummel
Update layout.html
725dfc7 
Period added.
oliverhummel
oliverhummel reviewed Jan 12, 2022
View reviewed changes
src/main/resources/templates/rooms/layout.html
@@ -28,8 +28,17 @@ <h1 style="margin-left: 0.42em; margin-top: 0.15em">
</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

@MorMundHS-MA I would assume that this checkout-bar div and its CSS definition in main.css can be savely removed, can't it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

If I understand the implementation correctly, the cookie handler adds the attribute to any request. In that case I assume this layout template should also have the checkout button since it doesn't use the template of the dashboard/landing page.

oliverhummel
oliverhummel reviewed Jan 12, 2022
View reviewed changes
src/main/java/de/hs_mannheim/informatik/ct/util/CookieManager.java
@@ -0,0 +1,151 @@
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

Didn't we manage cookies in a much simpler fashion before this? Can you briefly explain, why this is neccessary, pls?

oliverhummel
oliverhummel reviewed Jan 12, 2022
View reviewed changes
src/main/java/de/hs_mannheim/informatik/ct/web/WebConfig.java
@@ -0,0 +1,51 @@
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need an Interceptor here?

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

@oliverhummel oliverhummel oliverhummel left review comments

@MorMundHS-MA MorMundHS-MA MorMundHS-MA left review comments

@Gabril-E Gabril-E Awaiting requested review from Gabril-E

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Store a check-in in a cookie so that a check-out can happen from any page
4 participants
@maxones25 @Gabril-E @MorMundHS-MA @oliverhummel