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

Adds new features #23

Open
wants to merge 52 commits into
base: stable-3_3_0
Choose a base branch
from

Conversation

JhonathanLepidus
Copy link

This PR adds the following features:

  • Annotations page, where all submissions with annotations are listed
  • A new button with the number of annotations next to the PDF button in the preprint page. When this is clicked, the PDF is opened with the annotations tab opening automatically.
  • The same button to the preprint item in the list of preprints

These features were discussed in the issue

JhonathanLepidus and others added 30 commits July 20, 2022 16:38
It can now request the number of annotations of a galley to the Hypothesis API

Signed-off-by: Jhon <[email protected]>
Signed-off-by: Vitoria <[email protected]>
Also changes some visual styling

Signed-off-by: Jhon <[email protected]>
Also changes the path for annotations page

Signed-off-by: Jhon <[email protected]>
Signed-off-by: Jhon <[email protected]>
Signed-off-by: Richard <[email protected]>
Signed-off-by: Richard <[email protected]>
Signed-off-by: Jhon <[email protected]>
Signed-off-by: Jhon <[email protected]>
Signed-off-by: Richard <[email protected]>
Signed-off-by: Jhon <[email protected]>
Signed-off-by: Richard <[email protected]>
</div>
{/if}
<span class="annotation_content">
<blockquote>{$annotation->content}</blockquote>
Copy link
Owner

Choose a reason for hiding this comment

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

I'm not sure whether annotations should be allowed to contain limited HTML, but this should probably receive at least limited escaping for XSS attacks -- and probably also the other annotation attributes (user and target) too.

Copy link
Author

Choose a reason for hiding this comment

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

@asmecher
Copy link
Owner

Thanks, @JhonathanLepidus! @defstat, would you mind taking a deeper look?

@defstat
Copy link
Contributor

defstat commented Mar 6, 2024

@JhonathanLepidus two questions on this:

I can see that the description of the issue defines

A new button with the number of annotations next to the PDF button in the preprint page.

  1. I don't seem to see this button - should I do something in order to see it?
  2. Does that work for OPS (because you are referring to preprints) or it should work across applications, namely OJS, OMP and OPS?

@JhonathanLepidus
Copy link
Author

JhonathanLepidus commented Mar 11, 2024

@defstat sorry for missing this information. In fact, there should be an integration with the theme plugin in order for it to work. The integration is the calling of the hook Hypothesis::annotationNumber in the place where it is intended to display the button.

For example, we did this implementation in the SciELO Preprints theme: example. It should work in other applications besides OPS.

We did it this way because it was simpler for us, but I think it can be a problem for others to use. Do you think we should change it?

@JhonathanLepidus
Copy link
Author

We were able to identify an alternative implementation. We can change the plugin so that it uses template filters to add the button with the number of annotations next to the galley buttons.

This way, this feature could work on both platforms (OJS/OPS) without needing to be linked to a specific theme.

@defstat
Copy link
Contributor

defstat commented Mar 14, 2024

Hi @JhonathanLepidus. Thanks for that - Yes, the plugins should be "self contained", so your initial approach should be managed somehow. @jardakotesovec is a better person to talk about front end implementations, so I leave that for him.

Do you plan to migrate that change to stable-3.4 and/or main branch?

Also could you provide some screenshots on the intended end-result of the features this implementation adds?

@JhonathanLepidus
Copy link
Author

Hi! Yes, we plan to make these features available for the version compatible with stable 3.4.0 as well. Below are some screenshots of these features.

The annotations page:

annotations_page

Button with annotations number on archive page

archive_page

Button with annotations number on preprint page

preprint_page

Also removes dependency for others plugins

Signed-off-by: Jhon <[email protected]>
@JhonathanLepidus
Copy link
Author

Hi @defstat

We have updated the code in the source branch of this pull request, in order to bring the following improvements:

  • Updating of the cache cointaining the submissions with annotations through a scheduled task by the Acron plugin
  • The buttons with the number of annotations are now added without the use of a dependency with the theme plugin
  • Minor adjustements that make the new features fully compatible with both OJS and OPS

The features have also been adapted to the version compatible with OJS/OPS 3.4.0 and are ready to be shipped at a later moment.

@defstat
Copy link
Contributor

defstat commented Apr 2, 2024

Hi @JhonathanLepidus! Thanks for all that work! It looks great, and I see that you have adapted the newer/more fluent syntax regarding

Only one question about the caching - is the caching mechanism going to affect the realtime update of the annotation display?

If you have PRs for 3.4 version, please don't hesitate to add those in #22 so that they can be reviewed also.

@JhonathanLepidus
Copy link
Author

JhonathanLepidus commented Apr 2, 2024

Hi @defstat , thanks for the feedback.

The caching mechanism does not affect the display of annotations when viewing the PDF. The cached items are only used for display on the annotations page and do not affect other parts of the application.

We will open a pull request with our contributions to 3.4.0.

Edit: PR opened

@defstat
Copy link
Contributor

defstat commented Sep 24, 2024

@JhonathanLepidus thanks for all this work. Can you share the state of this change?

@JhonathanLepidus
Copy link
Author

HI @defstat !

The changes are ready to be merged. They're present in this pull request (for 3.3.0 version) and in this other one (for 3.4.0 version).

If you have any other doubt, please contact me.

Copy link
Contributor

@defstat defstat left a comment

Choose a reason for hiding this comment

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

Thanks @JhonathanLepidus so much for those changes, and I am sorry for delaying this. Just one or two comments here:

  1. I have tried to annotate something on a PDF and then to check that annotation on the interface, but it did not seem to be visible. Maybe I am doing something wrong, but if you can, please try to test the plugin's full functionality on stable-3_3_0 branch, with PHP 7.3 which is the oldest we support on stable-3_3_0.
  2. Please try the same on stable-3_4_0 for the other PR you are mentioning, and PHP 8.0 which is the oldest PHP version we support on that stable branch.

HypothesisPlugin.inc.php Outdated Show resolved Hide resolved
HypothesisPlugin.inc.php Outdated Show resolved Hide resolved
@JhonathanLepidus
Copy link
Author

Hi @defstat , thanks for the review of these changes.

I have tested both versions of these changes in PHP 7.3 and PHP 8.0. Some fixes were needed for the first, but the second worked well.

I also fixed one bug in the 3.3.0-compatible version regarding the scheduled task and solved the problem with the runScheduledTasks tool.

I formatted the PHP code using tabs in the 3.3.0 version. However, I noticed the plugin doesn't follow the PSR-12 standard in 3.4.0, so I didn't know what to do in this case.

@defstat
Copy link
Contributor

defstat commented Nov 15, 2024

Hi @JhonathanLepidus, because I have some functionality questions regarding this, could we set-up a quick demo meeting for the new features?

@JhonathanLepidus
Copy link
Author

Hi @defstat . Sure, I'm available for a meeting on any day of the week, except on wednesdays afternoons (considering brazilian time).

@defstat
Copy link
Contributor

defstat commented Nov 19, 2024

Thanks @JhonathanLepidus! My email is [email protected]. Send me when ever you can an email so that we can schedule something there.

@defstat
Copy link
Contributor

defstat commented Dec 18, 2024

Hi @JhonathanLepidus! do we have any development on this?

The bugs were regarding the retrieving of a group of annotations and submissions which used custom url paths

Signed-off-by: Jhon <[email protected]>
@JhonathanLepidus
Copy link
Author

Hi @defstat ! These changes were not firstly prioritized, but I found some time to do these tests. As we discussed, I did the tests with the PKP datasets. I found two bugs on the 3.3.0 and 3.4.0 changes.

When retrieving the annotations, the submissions were divided in groups and the Hypothesis API was queried for each group (to reduce time in large databases). The bug affected the last group queried. Since the PKP dataset has only two submissions, there was only one group, which was not queried.

The second was regarding submissions with a custom URL path. This bug was present only on the 3.3.0 changes.

I still did not implement the new item for the site's navigation menu. Please, let me know if you still have problems testing these features.

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.

6 participants