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 logic to retrieve omero.qa.feedback property server-side #327

Merged
merged 4 commits into from
Nov 25, 2022

Conversation

sbesson
Copy link
Member

@sbesson sbesson commented Nov 23, 2022

Companion to ome/omero-common#38

Similarly to the logic retrieving the maximum plane width/height from the server, this adds another post-connection step to retrieve the value of the server-side omero.qa.feedback property. If defined, it sets the LookupNames.TOKEN_URL and LookupNames.PROCESSING_URL entries which are used for sending feedback (comments and failed uploads) to the QA server. If the calls fails e.g. with a SecurityViolation, the current behavior is unchanged and the default values set in container.xml should be used.

Testing should happen in different client/server scenarios:

  • OMERO.insight with this PR included against OMERO.server without Define omero.qa.feedback as new visible server property omero-common#38:
    • if logged in as a normal user, the ConfigService call should throw an exception (logged in the OMERO.insight log file), the default value specified in container.xml should be used and the feedback should be sent to the OME QA system
    • if logged in as an admin, the ConfigService call should return an empty string (unless the value has been set) and ignored, the default value specified in container.xml should be used and the feedback should be sent to the OME QA system
  • OMERO.insight with this PR included against OMERO.server with Define omero.qa.feedback as new visible server property omero-common#38 included but not value defined for omero.qa.feedback server-side: the ConfigService should return the default value, the log file should indicate that Using URL defined server-side for feedback: http://qa.openmicroscopy.org.uk and the feedback should be sent to the OME QA system
  • (optional) OMERO.insight with this PR included against OMERO.server with Define omero.qa.feedback as new visible server property omero-common#38 included andomero.qa.feedback set server-side to http://qa.openmicroscopy.org.uk: the log file should indicate that Using URL defined server-side for feedback: http://qa.openmicroscopy.org.uk and the feedback should be sent to the OME QA system
  • OMERO.insight with this PR included against OMERO.server with Define omero.qa.feedback as new visible server property omero-common#38 included andomero.qa.feedback set server-side to an alternate QA system: the log file should indicate that Using URL defined server-side for feedback: <alternate QA endpoint> and the feedback should be sent to the alternate QA system

For each configuration, ideally both comments (Help > Send Feedback) and failed uploads should be tested.

If not null, generate feedback URLs and bind them to the TOKEN_URL and
PROCESSING_URL lookup names
@jburel jburel closed this Nov 24, 2022
@jburel jburel reopened this Nov 24, 2022
@jburel
Copy link
Member

jburel commented Nov 24, 2022

@sbesson I think I figured out where the problem is coming from when switching user.
Do we have a server already configured?

@sbesson
Copy link
Member Author

sbesson commented Nov 24, 2022

Do we have a server already configured?

I have a local server with my omero-common changes deployed and several test servers without the change

@jburel
Copy link
Member

jburel commented Nov 24, 2022

scenario 3: set omero.qa.feedback on merge-ci to http://qa.openmicroscopy.org.uk. Feedback received as expected see https://www.openmicroscopy.org/qa2/qa/feedback/31441/

@jburel
Copy link
Member

jburel commented Nov 24, 2022

scenario 4 works as expected too

@sbesson sbesson requested a review from jburel November 25, 2022 08:46
Copy link
Member

@jburel jburel left a comment

Choose a reason for hiding this comment

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

Changes look good. I will do a test for the switch user situation but the problem is not related to the proposed changes.

@jburel jburel merged commit adbbd05 into ome:master Nov 25, 2022
@jburel jburel mentioned this pull request Aug 21, 2024
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.

2 participants