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

New feature: shareable user query #6052

Merged
merged 51 commits into from
Feb 26, 2024
Merged

New feature: shareable user query #6052

merged 51 commits into from
Feb 26, 2024

Conversation

Alkarex
Copy link
Member

@Alkarex Alkarex commented Jan 21, 2024

Share the output of a user query by RSS / HTML / OPML with other people through unique URLs.

Replaces the global admin token, which was the only option (but unsafe) to share RSS outputs with other people.

Also add a new HTML output for people without an RSS reader.

fix #3066 (comment)
fix #3178 (comment)

image

Share the output of a user query by RSS / HTML / OPML with other people through unique URLs.
Replaces the global admin token, which was the only option (but unsafe) to share RSS outputs with other people.
Also add a new HTML output for people without an RSS reader.

fix FreshRSS#3066 (comment)
fix FreshRSS#3178 (comment)
@Alkarex Alkarex added Security 🛡️ API 🤝 API for other clients labels Jan 21, 2024
@Alkarex Alkarex added this to the 1.24.0 milestone Jan 21, 2024
This was referenced Jan 21, 2024
@Alkarex Alkarex mentioned this pull request Jan 23, 2024
@Alkarex Alkarex marked this pull request as ready for review January 25, 2024 07:49
@Alkarex
Copy link
Member Author

Alkarex commented Jan 25, 2024

Not completely done, but ready for tests and comments.
Lacking in particular:

  • checkbox to enable/disable sharing of a given user query (will also fix the fact that existing user queries need to be saved once before sharing works)
  • paging in HTML outputs
  • OPML sharing
  • Display user labels?
  • Fix article tags
  • Additional user search
  • a bit of documentation (help welcome)
  • HTTP Conditional requests (HTTP 304, caching)

@Alkarex
Copy link
Member Author

Alkarex commented Jan 25, 2024

I realised that the current implementation of the user queries was doing lots of redundant SQL queries, also when the user queries were not used. So I did some more refactoring to fix that.

@Alkarex
Copy link
Member Author

Alkarex commented Feb 25, 2024

I have addressed the latest details, so all done here for now (several improvements in particular of the HTML output view are left to future work).
Any comment? Otherwise I will merge shortly.

@Alkarex Alkarex merged commit 39cc1c1 into FreshRSS:edge Feb 26, 2024
2 checks passed
@Alkarex Alkarex deleted the share-query branch February 26, 2024 08:01
@Alkarex Alkarex linked an issue Feb 26, 2024 that may be closed by this pull request
Comment on lines +29 to +42
## Share your user queries

A prerequisite is that the FreshRSS API(s) must be enabled in FreshRSS authentication settings.

From the configuration page of the user queries,
it is possible to share the output of the user queries with external users,
in the formats HTML, RSS, and OPML:

![Share user query](../img/users/user-query-share.png)

> ℹ️ Note that the sharing as OPML is only available for user queries based on all feeds, a category, or a feed.
> Sharing by OPML is **not** available for queries based on user labels or favourites or important feeds,
> to avoid leaking some feed details in an unintended manner.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let us think to give a note here, that this feature will be available with 1.24 (it will note, that it is not yet available)

Comment on lines +26 to +53
<div class="form-group">
<div class="group-controls">
<label class="checkbox" for="shareRss">
<input type="checkbox" name="query[shareRss]" id="shareRss" value="1" <?= $this->query->shareRss() ? 'checked="checked"' : ''?> />
<?= _t('conf.query.filter.shareRss') ?>
</label>
<?php if ($this->query->sharedUrlRss() !== ''): ?>
<ul>
<li><a href="<?= $this->query->sharedUrlHtml() ?>"><?= _i('link') ?> <?= _t('conf.query.share.html') ?></a></li>
<li><a href="<?= $this->query->sharedUrlRss() ?>"><?= _i('link') ?> <?= _t('conf.query.share.rss') ?></a></li>
</ul>
<?php endif; ?>
</div>
<div class="group-controls">
<label class="checkbox" for="shareOpml">
<input type="checkbox" name="query[shareOpml]" id="shareOpml" value="1" <?= $this->query->shareOpml() && $this->query->safeForOpml() ? 'checked="checked"' : '' ?>
<?= $this->query->safeForOpml() ? '' : 'disabled="disabled"' ?> />
<?= _t('conf.query.filter.shareOpml') ?>
</label>
<?php if ($this->query->sharedUrlOpml() !== ''): ?>
<ul>
<li><a href="<?= $this->query->sharedUrlOpml() ?>"><?= _i('link') ?> <?= _t('conf.query.share.opml') ?></a></li>
</ul>
<?php endif; ?>
</div>
<p class="help"><?= _i('help') ?> <?= _t('conf.query.share.help') ?></a></p>
<p class="help"><?= _i('help') ?> <?= _t('conf.query.help') ?></a></p>
</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

could we add a check here, if the API is disabled?
grafik

(btw: the text in the settings needs an update, while it is not only used for the API)

Copy link
Member Author

@Alkarex Alkarex Feb 27, 2024

Choose a reason for hiding this comment

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

It is indeed now used for more than the mobile apps.
Note that WebSub, which is also an API, has its own internal option

@math-GH
Copy link
Contributor

math-GH commented Feb 27, 2024

HTML:

There is a pagination. When I click on "Next"
grafik

an "empty" page appears
grafik

@math-GH
Copy link
Contributor

math-GH commented Feb 27, 2024

There is no log entry if the user query is not available for sharing (Service Unavailable! nor if the token is wrong ('User query not found!')

@math-GH math-GH mentioned this pull request Feb 27, 2024
2 tasks
@Alkarex
Copy link
Member Author

Alkarex commented Feb 27, 2024

an "empty" page appears

Inded (Edit: Now that I think about it, what I did in the main page is to fetch 1 article more than demanded to know whether there is more or not)

There is no log entry if the user query is not available for sharing

Yes, this is by design as this can better be seen and reacted upon from the Web server logs (e.g. with Fail2Ban or similar) as I would rather avoid giving the possibility to an external client to too easily flood the FreshRSS user logs. We return different HTTP status codes (404, 422, 503...), which are visible in the Web server logs.

Alkarex added a commit to Alkarex/FreshRSS that referenced this pull request Mar 5, 2024
* OPML regression due to *shared user queries* (the XPath attributes were not exported anymore) FreshRSS#6052
* Add master token to HTML Meta RSS link and OPML link FreshRSS#6159 (reply in thread)
Alkarex added a commit that referenced this pull request Mar 5, 2024
* OPML regression due to *shared user queries* (the XPath attributes were not exported anymore) #6052
* Add master token to HTML Meta RSS link and OPML link #6159 (reply in thread)
@math-GH math-GH mentioned this pull request Mar 21, 2024
'state' => 'State',
'tags' => 'Display by label',
'type' => 'Type',
),
'get_all' => 'Display all articles',
'get_all_labels' => 'Display articles with any label',
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it obsolete? I cannot find any place where it is used

Copy link
Contributor

Choose a reason for hiding this comment

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

Strange, I cannot find all this "get_...." labels used somewhere....
@Alkarex Could you help me please?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is because they are used by string concatenation (which I would like to get rid of precisely because it makes validation and maintenance quite challenging). And there is another layer of property with another name for the getter, which makes it even more difficult to search:

<li class="item"><?= _t('conf.query.get_' . $query->getGetType(), $query->getGetName()) ?></li>

<?php if (!empty($remainingTags)): // more than 7 tags: show dropdown menu ?>
<li class="item tag">
<div class="dropdown">
<div id="dropdown-tags2-<?= $this->entry->id() ?>" class="dropdown-target"></div>
Copy link
Contributor

Choose a reason for hiding this comment

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

This makes an regression :/
If the tags are shown above and below an article the ID needs to be different from eachother (above: tags and below tags2).
With tags.html, It is always tags2. Any idea how to handle it?

ping @Alkarex

Copy link
Member Author

Choose a reason for hiding this comment

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

Quick suggestion: #6990

Copy link
Contributor

Choose a reason for hiding this comment

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

It would fix it but it does not make me happy because of the very large number that is added.

It would be useful if a variable could be set in normal.html (i.e. $foo = 1 for header tags and $foo = 2 for footer tags). And in tags.html $foo is used. But I cannot find a solution that makes it possible to have the variable setter in normal.phtml

Copy link
Member Author

Choose a reason for hiding this comment

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

What is the issue with the number? It is rarely used

Alkarex added a commit to Alkarex/FreshRSS that referenced this pull request Nov 11, 2024
@Alkarex Alkarex mentioned this pull request Nov 11, 2024
Alkarex added a commit that referenced this pull request Nov 25, 2024
* Fix tag ID uniqueness
fix #6052 (comment)

* Update app/views/helpers/index/tags.phtml

Co-authored-by: maTh <[email protected]>

---------

Co-authored-by: maTh <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API 🤝 API for other clients Security 🛡️
Projects
None yet
3 participants