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

fix(website): Handle virtual DocTypes webview routing #24445

Merged
merged 1 commit into from
Feb 5, 2024

Conversation

cogk
Copy link
Contributor

@cogk cogk commented Jan 18, 2024

Virtual DocType × WebsiteGenerator = ❤️

Let's say that we have a virtual DocType Remote Blog Post which fetches data from another website (e.g. Airtable, Google Sheets, Nextcloud), which is also a WebsiteGenerator. The Remote Blog Post has a route field, and a published field.

Because the document does not exist in the database, it's not possible to find it using frappe.db.get_value. Instead, we have to use frappe.get_list (or something better, I'm open to suggestions).

An alternative is to add a get_docname_for_route method to WebsiteGenerator but this feels ugly.

@cogk cogk requested review from a team and shariquerik and removed request for a team January 18, 2024 11:06
Copy link

codecov bot commented Jan 18, 2024

Codecov Report

Attention: 52 lines in your changes are missing coverage. Please review.

Comparison is base (2b3ab02) 62.00% compared to head (02cc27b) 61.24%.
Report is 18 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #24445      +/-   ##
===========================================
- Coverage    62.00%   61.24%   -0.76%     
===========================================
  Files          786      786              
  Lines        75071    77511    +2440     
  Branches      6423     6417       -6     
===========================================
+ Hits         46548    47472     +924     
- Misses       24828    26378    +1550     
+ Partials      3695     3661      -34     
Flag Coverage Δ
server 70.91% <16.12%> (-0.05%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

@cogk cogk marked this pull request as draft January 18, 2024 11:54
@cogk

This comment was marked as outdated.

Co-authored-by: Charles-Henri Decultot <chdecultot@dokos.io>
@cogk
Copy link
Contributor Author

cogk commented Jan 19, 2024

In this PR I did NOT introduce a controller method to map a route to a document (e.g. get_docname_for_route(route: str)). However, this means that developers have to:

  1. Have a well designed get_list controller method.
  2. Take care of caching results to avoid unnecessary side effects (network requests, computations).

We might possibly have to consider a refactor of this method to call the hypothetical controller's get_docname_for_route method for fast querying. This means factoring out the whole frappe.db.get_value, leading to:

class WebsiteGenerator:
  @classmethod
  def get_docname_for_route(cls, route: str):
    return frappe.db.get_value(...)

@cogk cogk marked this pull request as ready for review January 19, 2024 18:27
@cogk
Copy link
Contributor Author

cogk commented Jan 20, 2024

Or maybe developers should just make a page in their www and handle things there. So we should disallow a Virtual DocType to also be a WebsiteGenerator ?

@ankush
Copy link
Member

ankush commented Feb 5, 2024

Take care of caching results to avoid unnecessary side effects (network requests, computations).

Framework does it for web view routing. Once route is resolved it stays cached for a while. I'd still avoid this TBH. Just writing plain pages which can use any doctype or external sources achieves the same results (?)

@ankush ankush merged commit c9c8414 into frappe:develop Feb 5, 2024
24 of 25 checks passed
@ankush ankush added the backport version-15-hotfix Backport the PR to v15 label Feb 5, 2024
mergify bot pushed a commit that referenced this pull request Feb 5, 2024
Co-authored-by: Charles-Henri Decultot <chdecultot@dokos.io>
(cherry picked from commit c9c8414)
ankush pushed a commit that referenced this pull request Feb 5, 2024
Co-authored-by: Charles-Henri Decultot <chdecultot@dokos.io>
(cherry picked from commit c9c8414)

Co-authored-by: Corentin Flr <10946971+cogk@users.noreply.github.com>
frappe-pr-bot pushed a commit that referenced this pull request Feb 6, 2024
# [15.13.0](v15.12.0...v15.13.0) (2024-02-06)

### Bug Fixes

* Allow int fields to be unique ([#24750](#24750)) ([#24756](#24756)) ([6c5ea2f](6c5ea2f))
* Avoid enqueueing during install ([#24679](#24679)) ([#24682](#24682)) ([7825a72](7825a72))
* Check if header/footer html exists before trying to extract ([03fc5b1](03fc5b1))
* check is_translatable link ([#24739](#24739)) ([#24747](#24747)) ([e21e2ca](e21e2ca))
* clear sitemap cache periodically ([#24676](#24676)) ([282e0e6](282e0e6))
* console import progress off-by-one ([#24777](#24777)) ([#24787](#24787)) ([e6ad6e8](e6ad6e8))
* Custom Script runs twice ([5d7d7f7](5d7d7f7))
* Dashboard view realtime error ([#24698](#24698)) ([#24699](#24699)) ([a2a766b](a2a766b))
* data import table UI fix ([064aca2](064aca2))
* date format & pre commit test ([9726c46](9726c46))
* depends on ([9777fe3](9777fe3))
* description ([fe7ed4b](fe7ed4b))
* Disappearing letterhead header in pdf ([e932958](e932958))
* do not allow to set if_owner & report perm together ([290b7e8](290b7e8))
* don't notify links if not public ([019c223](019c223))
* enable syntax highlighting for `PythonExpression` code fields ([#24669](#24669)) ([#24671](#24671)) ([4bbfaa7](4bbfaa7))
* Enqueue deletion of dynamic link after comitting ([#24675](#24675)) ([9fbe981](9fbe981))
* Extract header/footer html into `content` to simplify things ([64102a7](64102a7))
* formatting ([05a6ad8](05a6ad8))
* formatting ([5ba9be8](5ba9be8))
* hide number counter for new document view shortcut ([bb46d23](bb46d23))
* **js:** Handle hidden virtual fields ([#24405](#24405)) ([#24692](#24692)) ([ea82c81](ea82c81))
* **login:** Escape translated strings ([#24431](#24431)) ([#24701](#24701)) ([73c66ce](73c66ce))
* lower default retention periods ([#24697](#24697)) ([#24705](#24705)) ([a155f9e](a155f9e))
* make rate_limiter respect multitenancy ([#24634](#24634)) ([d25bfd9](d25bfd9))
* Make sure sitemap respects robot_txt ([10b583b](10b583b))
* never show virtual fields in list view (backport [#24666](#24666)) ([#24668](#24668)) ([45e2683](45e2683))
* no of rows displayed based on report type ([db4d36f](db4d36f))
* outgoing email account handlng (backport [#24656](#24656)) ([#24657](#24657)) ([4a59a01](4a59a01))
* redirect and open new doctype dialog if route to /doctype/new ([fec821a](fec821a))
* Retry contact update if it fails due to conflict ([#24654](#24654)) ([#24655](#24655)) ([22aa5d3](22aa5d3))
* Set default for search result limit if empty ([#24713](#24713)) ([#24715](#24715)) ([474feb8](474feb8))
* short circuit private files perm check ([e6d7ffe](e6d7ffe))
* show folders in Google Drive Picker ([0d99ef8](0d99ef8)), closes [#23096](#23096)
* Show proper error message for prepared report failure ([#24733](#24733)) ([#24737](#24737)) ([99676af](99676af))
* Skip fulltext indexes during sync ([#24728](#24728)) ([#24734](#24734)) ([8e078f3](8e078f3))
* suggestion ([63764a9](63764a9))
* title link check ([#24731](#24731)) ([#24735](#24735)) ([5c006cd](5c006cd))
* Translate Letterhead information string ([8dfa972](8dfa972))
* typeerror in onboarding_tours.js ([5ae79be](5ae79be))
* Use `TEXT` type for Webhook URLs to support large URLs ([#24763](#24763)) ([f31a3ce](f31a3ce))
* **UX:** improve server script message ([#24770](#24770)) ([5d88275](5d88275))
* virtual fields are never writable ([#24693](#24693)) ([#24696](#24696)) ([d020771](d020771))
* **website:** Handle virtual DocTypes webview routing ([#24445](#24445)) ([#24743](#24743)) ([bed66ca](bed66ca))

### Features

* allow extending site config with a hook ([ffed7bf](ffed7bf))
* Letterhead scripts ([9b296cd](9b296cd))
* **recorder:** profiling and granular recording (backport [#24683](#24683)) ([#24742](#24742)) ([fd04617](fd04617))
* show doctype description on list title hover (backport [#24612](#24612)) ([#24769](#24769)) ([5a0e20b](5a0e20b))
* show doctype description on workspace link hover ([#24598](#24598)) ([#24789](#24789)) ([6107338](6107338))
* use_first_day_of_period ([1fcb105](1fcb105))

### Performance Improvements

* optional faster perm check for files ([3e4a344](3e4a344))
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport version-15-hotfix Backport the PR to v15
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants