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

Support Form: math challenge doesn't work from some pages #3036

Closed
pdurbin opened this issue Mar 28, 2016 · 30 comments
Closed

Support Form: math challenge doesn't work from some pages #3036

pdurbin opened this issue Mar 28, 2016 · 30 comments
Labels
Type: Bug a defect UX & UI: Design This issue needs input on the design of the UI and from the product owner

Comments

@pdurbin
Copy link
Member

pdurbin commented Mar 28, 2016

In #1327 we added a simple math challenge to slow spammers down from filling out the support form but in some places the operands are now missing. For example on https://demo.dataverse.org running v. 4.3 build 22-1e451ff if you sign up, click the "welcome" notification, and click "Dataverse Support", you don't see any number to add together:

account_-demo_dataverse-_2016-03-28_09 01 11

As noted by @kcondon this regression appears in additional places in the Shib-related pull request at #3025 as of 4d332e0:

Interestingly, if you click the normal "Support" link in the header first, the problem goes away. From a quick look it seems like the problem started happening when we staring using constructions like <a href="#" onclick="event.preventDefault();PF('contactForm').show();">Dataverse Support</a> in Bundle.properties. I'm not actually sure what the fix should be. Here's a list of where this construction is used in pull request #3025:

murphy:dataverse pdurbin$ grep contactForm src/main/java/Bundle.properties | grep -i onclick | sed G

user.isShibUser=Account information cannot be edited when logged in through an institutional account. Leaving your institution? Please contact <a href="#" onclick="event.preventDefault();PF('contactForm').show();">Dataverse Support</a> for assistance.

notification.welcome=Welcome to {0} Dataverse! Get started by adding or finding data. Have questions? Check out the <a href="{1}/{2}/user/index.html" title="{0} Dataverse User Guide" target="_blank">User Guide</a> or contact <a href="#" onclick="event.preventDefault();PF(&#39;contactForm&#39;).show();" title=\u201dContact {0} Dataverse Support\u201d>Dataverse Support</a> for assistance.

login.institution.support=Leaving your institution? Please contact <a href="#" onclick="event.preventDefault();PF('contactForm').show();">Dataverse Support</a> for assistance.

error.404.message=<strong>Page Not Found</strong> - The page you are looking for was not found. If you believe this is an error, please contact <a href="#" onclick="event.preventDefault();PF('contactForm').show();">Dataverse Support</a> for assistance.

error.403.message=<strong>Not Authorized</strong> - You are not authorized to view this page. If you believe this is an error, please contact <a href="#" onclick="event.preventDefault();PF('contactForm').show();">Dataverse Support</a> for assistance.
@pdurbin pdurbin added the Type: Bug a defect label Mar 28, 2016
@pdurbin pdurbin changed the title Support Form: math challenge doesn't work in some cases Support Form: math challenge doesn't work from some pages Mar 28, 2016
@pdurbin
Copy link
Member Author

pdurbin commented Mar 28, 2016

@scolapasta thanks for agreeing during this morning's standup to take a look at this issue. Again, the easiest fix is to remove the onclick ... PF stuff from https://github.com/IQSS/dataverse/blob/v4.3/src/main/java/Bundle.properties#L129 for example and have it say "Click the link in the header" but maybe you can come up with a better fix. @landreev seems to agree that PrimeFaces code doesn't really belong in the bundle and if that's the case maybe we should put this in our style guide: http://guides.dataverse.org/en/4.3/developers/coding-style.html

@scolapasta during the meeting I said I'd also give two more issues to you that were kicked back to me. Here are links to the comments from @kcondon:

Since the 2939-shib branch that pull request #3025 is based on is particularly effected, I would suggest fixing the problem there.

@pdurbin pdurbin added this to the 4.4 milestone Mar 28, 2016
@scolapasta
Copy link
Contributor

Giving this to @mheppler. In 4.3 for the notification, and in the shib branch for all the other support references, he added these links to Support. (403 and 404 used to say, click support link on the header).

However these references in the bundle only open up the dialog box and don't do what happens when you click on the link in the header, i.e call to the server to initialize the popup.

So (and @pdurbin mentions some of this above we need to:
a) either go back to just saying "please click link in header"
b) find javascript that will actually click the link as if the user clicked in
c)put the logic on each of the links on the 403, 404, shib login, and account pages to initialize.

The notification is a different case. In particular, we do also send out notification texts in e-mail. So we may not want to have this in the first place, as you're not in the app, i.e. can't open up a link.

@scolapasta scolapasta assigned mheppler and unassigned scolapasta Mar 28, 2016
@scolapasta
Copy link
Contributor

Here is the code from the header:

                    <h:form class="navbar-form navbar-left navbar-form-link">
                       <p:commandLink value="#{bundle['header.support']}" oncomplete="PF('contactForm').show()" update=":contactDialog" actionListener="#{sendFeedbackDialog.initUserInput}">
                            <f:setPropertyActionListener target="#{sendFeedbackDialog.messageSubject}" value=""/>
                            <f:setPropertyActionListener target="#{sendFeedbackDialog.recipient}" value="#{null}"/>
                            <f:setPropertyActionListener target="#{sendFeedbackDialog.userMessage}" value=""/>
                            <f:setPropertyActionListener target="#{sendFeedbackDialog.userEmail}" value=""/>
                       </p:commandLink>
                    </h:form>

In particular, some of the properties would be set differently depending on where you clicked the page from, so fir example, we could prepopulate messageSubject to be "404" if from that page.

This, of course, would suggest that c above would be the course of action.

@mheppler
Copy link
Contributor

After reviewing all the comments and looking at the code, I believe that best of the three options outlined above is "c) put the logic on each of the links on the 403, 404, shib login, and account pages to initialize".

Passing this ticket to Gustavo to implement this or better yet find an even more dynamic solution that would allow bundle messages with contact support links to be easily adding going forward, without needing a custom solution.

@mheppler mheppler assigned scolapasta and unassigned mheppler Mar 30, 2016
@eaquigley
Copy link
Contributor

+1 to @mheppler's comment. I do not think it should be changed to "click the link in the header". Would not be a good user experience practice.

@pdurbin
Copy link
Member Author

pdurbin commented Apr 19, 2016

When @scolapasta gave this issue to me the other day we discussed it and decided to

  • Revert the 403 and 404 pages back to how they are in 4.3 (no links to support) because @scolapasta will be working on this part of the code in the widgets branch - done in 656a074
  • Remove all PF code from the bundle - done in adf1e34

In total there are 5 places to check:

  • 403 page
  • 404 page
  • notification on signup
  • Login page when Shib is enabled (math question should now work)
  • Account page for Shib user (math question should now work)

I used my best judgement to fix the notification regression in 4.3 that was introduced in #2966 (that issue should be tested or retested). Here's a before and after:

Notification Before

screen shot 2016-04-19 at 1 35 53 pm

Notification After

screen shot 2016-04-19 at 1 59 35 pm

Passing to QA.

@pdurbin pdurbin assigned kcondon and unassigned pdurbin Apr 19, 2016
@eaquigley
Copy link
Contributor

Discussed with @pdurbin and to avoid dead ends with the Support text, he will change the text to say, "or email (insert system email address link) for assistance."

@eaquigley eaquigley assigned pdurbin and unassigned kcondon Apr 19, 2016
@pdurbin pdurbin removed their assignment Jan 17, 2017
@pdurbin
Copy link
Member Author

pdurbin commented Jan 17, 2017

I'm happy with the code in the 3036-support-form-math-bug so I went ahead and created pull request #3573 based on it. I did the following:

Note that I could not test the math bug scenario mentioned in #3265 because I got a proper error message when I tried to use more than 30 characters for a client nickname.

In short, I believe the math challenge now works everywhere. If there's a place where it doesn't work, I don't know where it is.

Passing to QA.

@kcondon
Copy link
Contributor

kcondon commented Jan 23, 2017

Tested:

  1. Main nav bar
  2. 403, 404.xhtml pages
  3. Shib enabled log in page
  4. Caught exception page (2x role assign)
  5. Email verification expire message
  6. Log in no user name error message.

This appears to be working. Closing.

@kcondon
Copy link
Contributor

kcondon commented Jan 23, 2017

@pdurbin Works but needs to have merge conflicts resolved.

@kcondon kcondon assigned kcondon and unassigned pdurbin Jan 23, 2017
@kcondon
Copy link
Contributor

kcondon commented Jan 24, 2017

All set. Closing.

@djbrooke
Copy link
Contributor

This was pulled back into the development column because the fix broke publish. This has been fixed, but a different approach will need to be taken here.

@djbrooke
Copy link
Contributor

I'm moving this into the backlog. We'll address this in a future sprint.

@djbrooke djbrooke removed this from the 4.6.1 - ORCID and File Replace milestone Feb 7, 2017
@pdurbin pdurbin added the UX & UI: Design This issue needs input on the design of the UI and from the product owner label Jun 8, 2017
@pdurbin
Copy link
Member Author

pdurbin commented Jun 28, 2017

I'm moving this into the backlog. We'll address this in a future sprint.

This issue goes on and on. Let's create a fresh one when we're ready to estimate it. Closing.

@pdurbin pdurbin closed this as completed Jun 28, 2017
@kcondon
Copy link
Contributor

kcondon commented Aug 29, 2019

This is still an issue, encountered when testing: #6136
If folks want a shorter issue, can reopen this one: #3265

@kcondon kcondon reopened this Aug 29, 2019
@pdurbin
Copy link
Member Author

pdurbin commented Oct 23, 2019

Closing in favor of #6307

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug a defect UX & UI: Design This issue needs input on the design of the UI and from the product owner
Projects
None yet
Development

No branches or pull requests

6 participants