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

remove session.setUser(pre-save user) on email change #8643 #8644

Merged
merged 1 commit into from
Apr 25, 2022

Conversation

pdurbin
Copy link
Member

@pdurbin pdurbin commented Apr 22, 2022

What this PR does / why we need it:

When you change your email, the tabs on the user page are blank.

Which issue(s) this PR closes:

Special notes for your reviewer:

I'm not sure why session.setUser(currentUser) is here (it was added in b13f9be) and removing it seems to help.

There are still weird messages in server.log but fewer than the stacktrace seen in #8643.

When you click "Save Changes":

[2022-04-22T14:40:23.267-0400] [Payara 5.2021.5] [WARNING] [jsf.externalcontext.flash.response.already.committed] [javax.enterprise.resource.webcontainer.jsf.flash] [tid: _ThreadID=116 _ThreadName=http-thread-pool::http-listener-1(1)] [timeMillis: 1650652823267] [levelValue: 900] [[
JSF1095: The response was already committed by the time we tried to set the outgoing cookie for the flash. Any values stored to the flash will not be available on the next request.]]

When you click another tab and then go back to "Account Information":

[2022-04-22T14:40:46.258-0400] [Payara 5.2021.5] [WARNING] [] [] [tid: _ThreadID=116 _ThreadName=http-thread-pool::http-listener-1(1)] [timeMillis: 1650652846258] [levelValue: 900] [[
Response has already been committed, and further write operations are not permitted. This may result in an IllegalStateException being triggered by the underlying application. To avoid this situation, consider adding a Rule .when(Direction.isInbound().and(Response.isCommitted())).perform(Lifecycle.abort()), or figure out where the response is being incorrectly committed and correct the bug in the offending code.]]

Suggestions on how to test this:

#8643 contains steps.

Does this PR introduce a user interface change? If mockups are available, please link/include them here:

Yes, instead of going to "My Data"on email change, you stay on "Account Information" which looks like this:

Screen Shot 2022-04-22 at 2 45 03 PM

Is there a release notes update needed for this change?:

No.

Additional documentation:

None.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.0003%) to 18.969% when pulling 124c4b2 on 8643-email-change into 2c805a7 on develop.

@@ -384,7 +384,6 @@ public String save() {
} catch (ConfirmEmailException ex) {
logger.log(Level.INFO, "Unable to send email confirmation link to user id {0}", savedUser.getId());
}
session.setUser(currentUser);
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense here to setUser to the savedUser from line 372?

Copy link
Member Author

Choose a reason for hiding this comment

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

I actually tried this first but didn't take good notes. Using savedUser doesn't help.

When you save the email change you get this in server.log and go to "My Data"

[2022-04-25T11:58:34.093-0400] [Payara 5.2021.5] [WARNING] [jsf.externalcontext.flash.response.already.committed] [javax.enterprise.resource.webcontainer.jsf.flash] [tid: _ThreadID=121 _ThreadName=http-thread-pool::http-listener-1(5)] [timeMillis: 1650902314093] [levelValue: 900] [[
JSF1095: The response was already committed by the time we tried to set the outgoing cookie for the flash. Any values stored to the flash will not be available on the next request.]]

Screen Shot 2022-04-25 at 11 59 03 AM

Then, if you click "Account Information" you get a stacktrace and and empty tab:

[2022-04-25T11:59:15.297-0400] [Payara 5.2021.5] [SEVERE] [] [javax.enterprise.resource.webcontainer.jsf.context] [tid: _ThreadID=121 _ThreadName=http-thread-pool::http-listener-1(5)] [timeMillis: 1650902355297] [levelValue: 1000] [[
java.lang.IndexOutOfBoundsException: Index 2 out of bounds for length 2

Screen Shot 2022-04-25 at 11 59 28 AM

@kcondon kcondon self-assigned this Apr 25, 2022
@kcondon kcondon merged commit e713194 into develop Apr 25, 2022
@kcondon kcondon deleted the 8643-email-change branch April 25, 2022 18:36
@pdurbin pdurbin added this to the 5.11 milestone May 4, 2022
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.

After email change, blank Account Information, Notifications, and API Token tabs
4 participants