Resolve "Review and fix/improve PWA behaviour and serviceworker"

yuha commented

2020-07-23 00:15:11 +02:00

Member

Closes #230

yuha commented

2020-07-24 01:07:47 +02:00

Author

Member

added 1 commit

a8d36850 - Add serviceworker template, settings and file replacement functionality

Compare with previous version

added 1 commit <ul><li>a8d36850 - Add serviceworker template, settings and file replacement functionality</li></ul> [Compare with previous version](/AlekSIS/official/AlekSIS/-/merge_requests/336/diffs?diff_id=2877&start_sha=c0331a18294a365d9d4f6ca384ab500b52a492c3)

yuha commented

2020-07-24 14:05:28 +02:00

Author

Member

added 2 commits

b310a267 - Add settings
5298322f - Use regexes

Compare with previous version

added 2 commits <ul><li>b310a267 - Add settings</li><li>5298322f - Use regexes</li></ul> [Compare with previous version](/AlekSIS/official/AlekSIS/-/merge_requests/336/diffs?diff_id=2881&start_sha=a8d3685085a0fc1ad630e898ed6b00be1d9874a5)

yuha commented

2020-07-24 15:18:04 +02:00

Author

Member

unmarked as a Work In Progress

unmarked as a **Work In Progress**

yuha commented

2020-07-24 15:18:18 +02:00

Author

Member

Finished and can be merged.

yuha commented

2020-08-01 16:40:12 +02:00

Author

Member

added 1 commit

927bd262 - Fixed offline page bug

Compare with previous version

added 1 commit <ul><li>927bd262 - Fixed offline page bug</li></ul> [Compare with previous version](/AlekSIS/official/AlekSIS/-/merge_requests/336/diffs?diff_id=2931&start_sha=5298322f1cf5533d7dcb0d4ffc18569a671a9433)

yuha commented

2020-08-01 20:57:26 +02:00

Author

Member

added 11 commits

927bd262...4e6ceb0f - 10 commits from branch master
623bf2f6 - Merge branch 'master' into 230-review-and-fix-improve-pwa-behaviour-and-serviceworker

Compare with previous version

added 11 commits <ul><li>927bd262...4e6ceb0f - 10 commits from branch <code>master</code></li><li>623bf2f6 - Merge branch 'master' into 230-review-and-fix-improve-pwa-behaviour-and-serviceworker</li></ul> [Compare with previous version](/AlekSIS/official/AlekSIS/-/merge_requests/336/diffs?diff_id=2941&start_sha=927bd2622c8f1b5a436597a53f94780b2a29b5d5)

yuha commented

2020-08-01 21:08:26 +02:00

Author

Member

added 1 commit

aded519a - Add descriptions

Compare with previous version

added 1 commit <ul><li>aded519a - Add descriptions</li></ul> [Compare with previous version](/AlekSIS/official/AlekSIS/-/merge_requests/336/diffs?diff_id=2942&start_sha=623bf2f6c8a35c04b7d836987f216abc2702bc3a)

nik commented

2020-08-07 16:36:23 +02:00

Owner

Please use a context wrapper here.

nik commented

2020-08-07 16:42:31 +02:00

Owner

marked as a Work In Progress

marked as a **Work In Progress**

yuha commented

2020-10-31 17:28:19 +01:00

Author

Member

Could you please explain this?

hansegucker commented

2020-10-31 17:29:05 +01:00

Owner

        with open(settings.PWA_SERVICE_WORKER_PATH, "w") as serviceworker:

```suggestion:-0+0 with open(settings.PWA_SERVICE_WORKER_PATH, "w") as serviceworker: ```

hansegucker commented

2020-11-01 12:46:30 +01:00

Owner

        with open(settings.PWA_SERVICE_WORKER_PATH, "w") as serviceworker:
            serviceworker.write(rendered)

```suggestion:-2+0 with open(settings.PWA_SERVICE_WORKER_PATH, "w") as serviceworker: serviceworker.write(rendered) ```

yuha commented

2020-11-21 12:33:30 +01:00

Author

Member

changed this line in version 7 of the diff

changed this line in [version 7 of the diff](/AlekSIS/official/AlekSIS-Core/-/merge_requests/336/diffs?diff_id=3889&start_sha=aded519a4e36188bfb3be924184dccfac103eb38#c5bff67ef2f26802f3c80e9bf77dc3105d8fc8ec_62_62)

yuha commented

2020-11-21 12:33:32 +01:00

Author

Member

added 1 commit

d64997e1 - Apply 1 suggestion(s) to 1 file(s)

Compare with previous version

added 1 commit <ul><li>d64997e1 - Apply 1 suggestion(s) to 1 file(s)</li></ul> [Compare with previous version](/AlekSIS/official/AlekSIS-Core/-/merge_requests/336/diffs?diff_id=3889&start_sha=aded519a4e36188bfb3be924184dccfac103eb38)

yuha commented

2020-11-21 12:38:03 +01:00

Author

Member

added 203 commits

d64997e1...0ff3ebb4 - 201 commits from branch master
73de29fe - Merge branch 'master' into 230-review-and-fix-improve-pwa-behaviour-and-serviceworker
884404bc - Merge remote-tracking branch...

Compare with previous version

added 203 commits <ul><li>d64997e1...0ff3ebb4 - 201 commits from branch <code>master</code></li><li>73de29fe - Merge branch 'master' into 230-review-and-fix-improve-pwa-behaviour-and-serviceworker</li><li>884404bc - Merge remote-tracking branch...</li></ul> [Compare with previous version](/AlekSIS/official/AlekSIS-Core/-/merge_requests/336/diffs?diff_id=3892&start_sha=d64997e1c931bec0a88eafe1d21bd23d419bf03a)

yuha commented

2020-11-21 12:39:40 +01:00

Author

Member

changed this line in version 9 of the diff

changed this line in [version 9 of the diff](/AlekSIS/official/AlekSIS-Core/-/merge_requests/336/diffs?diff_id=3894&start_sha=884404bc20526f3c5b2d4ce64b42a7374a39d4e2#c5bff67ef2f26802f3c80e9bf77dc3105d8fc8ec_64_63)

yuha commented

2020-11-21 12:39:41 +01:00

Author

Member

added 1 commit

f2310371 - Apply suggestion

Compare with previous version

added 1 commit <ul><li>f2310371 - Apply suggestion</li></ul> [Compare with previous version](/AlekSIS/official/AlekSIS-Core/-/merge_requests/336/diffs?diff_id=3894&start_sha=884404bc20526f3c5b2d4ce64b42a7374a39d4e2)

yuha commented

2020-11-21 12:41:40 +01:00

Author

Member

added 1 commit

ee055874 - Use context wrapper when writing serviceworker

Compare with previous version

added 1 commit <ul><li>ee055874 - Use context wrapper when writing serviceworker</li></ul> [Compare with previous version](/AlekSIS/official/AlekSIS-Core/-/merge_requests/336/diffs?diff_id=3895&start_sha=f231037163318a541540d1a18485fd7b366f9b96)

yuha commented

2020-11-21 12:41:58 +01:00

Author

Member

resolved all threads

hansegucker commented

2020-11-21 12:46:38 +01:00

Owner

assigned to @nik

yuha commented

2020-11-21 12:59:51 +01:00

Author

Member

added 1 commit

f8d1ecc6 - Fixed typo which prevented offline page to load

Compare with previous version

added 1 commit <ul><li>f8d1ecc6 - Fixed typo which prevented offline page to load</li></ul> [Compare with previous version](/AlekSIS/official/AlekSIS-Core/-/merge_requests/336/diffs?diff_id=3896&start_sha=ee0558748dc863ccc56ddc40d75fda5014f14fdb)

yuha commented

2020-11-21 13:05:22 +01:00

Author

Member

unmarked as a Work In Progress

unmarked as a **Work In Progress**

hansegucker commented

2020-11-21 16:29:33 +01:00

Owner

added 1 commit

af2e20c2 - Reformat

Compare with previous version

added 1 commit <ul><li>af2e20c2 - Reformat</li></ul> [Compare with previous version](/AlekSIS/official/AlekSIS-Core/-/merge_requests/336/diffs?diff_id=3905&start_sha=f8d1ecc6ba4e45424b2b5fb1d820265d29cb07a4)

nik commented

2020-11-21 18:34:19 +01:00

Owner

Iff these regexes indeed match anywhere in the request path, these will break all requests that contain these words anywhere.

nik commented

2020-12-04 23:20:42 +01:00

Owner

marked this merge request as draft

marked this merge request as **draft**

yuha commented

2020-12-05 15:38:54 +01:00

Author

Member

changed this line in version 13 of the diff

changed this line in [version 13 of the diff](/AlekSIS/official/AlekSIS-Core/-/merge_requests/336/diffs?diff_id=3998&start_sha=af2e20c2f39fb4b55d6723619146cc0e71728ebe#abc9d828b78bd9d36e05475d0bc91119bc5bc6b0_541_535)

yuha commented

2020-12-05 15:38:55 +01:00

Author

Member

added 2 commits

27728b0f - Remove serviceworker in static
9f4d44a9 - Read headers in SW to decide whether to cache a page or not (via cache-control)

Compare with previous version

added 2 commits <ul><li>27728b0f - Remove serviceworker in static</li><li>9f4d44a9 - Read headers in SW to decide whether to cache a page or not (via cache-control)</li></ul> [Compare with previous version](/AlekSIS/official/AlekSIS-Core/-/merge_requests/336/diffs?diff_id=3998&start_sha=af2e20c2f39fb4b55d6723619146cc0e71728ebe)

yuha commented

2020-12-05 16:02:44 +01:00

Author

Member

added 1 commit

d52b117b - Add never_cache decorator for several views

Compare with previous version

added 1 commit <ul><li>d52b117b - Add never_cache decorator for several views</li></ul> [Compare with previous version](/AlekSIS/official/AlekSIS-Core/-/merge_requests/336/diffs?diff_id=3999&start_sha=9f4d44a9c34ba082777ef5df9046f51ffd96ebe8)

yuha commented

2020-12-05 16:04:57 +01:00

Author

Member

resolved all threads

yuha commented

2020-12-05 16:05:37 +01:00

Author

Member

So I guess I made a mistake; the serviceworker can indeed access the response headers.

yuha commented

2020-12-05 16:05:56 +01:00

Author

Member

This can be merged now.

yuha commented

2020-12-05 16:06:01 +01:00

Author

Member

marked this merge request as ready

marked this merge request as **ready**

yuha commented

2020-12-05 16:06:08 +01:00

Author

Member

resolved all threads

nik commented

2020-12-05 18:05:26 +01:00

Owner

STATIC_ROOT is not guaranteed to be writable at runtime, so as explained before, this should happen when running collectstatic.

I also do not see why cache name and fallback path should be configurable, and precache paths are also unnecessary (even more so, given that the default, which includes /, is wrong, as / is not guaranteed to be the dashboard, and even now is not the dashboard under all conditions — consider that precaching happens before the first login, so if this precaches anything, it precaches a redirect or the login page; and after login, the dashboard is cached hen loading even without leveraging precaching).

So to conlude — just drop all of that and make the service worker static.

`STATIC_ROOT` is not guaranteed to be writable at runtime, so as explained before, this should happen when running `collectstatic`. I also do not see why cache name and fallback path should be configurable, and precache paths are also unnecessary (even more so, given that the default, which includes `/`, is wrong, as `/` is not guaranteed to be the dashboard, and even now is not the dashboard under all conditions — consider that precaching happens *before* the first login, so if this precaches anything, it precaches a redirect or the login page; and after login, the dashboard is cached hen loading even without leveraging precaching). So to conlude — just drop all of that and make the service worker static.

yuha commented

2020-12-05 19:04:41 +01:00

Author

Member

changed this line in version 15 of the diff

changed this line in [version 15 of the diff](/AlekSIS/official/AlekSIS-Core/-/merge_requests/336/diffs?diff_id=4002&start_sha=d52b117b79cb43a6f4f470be47e009575f124325#c5bff67ef2f26802f3c80e9bf77dc3105d8fc8ec_61_54)

yuha commented

2020-12-05 19:04:43 +01:00

Author

Member

added 1 commit

ce78324e - Remove dynamic serviceworker generation

Compare with previous version

added 1 commit <ul><li>ce78324e - Remove dynamic serviceworker generation</li></ul> [Compare with previous version](/AlekSIS/official/AlekSIS-Core/-/merge_requests/336/diffs?diff_id=4002&start_sha=d52b117b79cb43a6f4f470be47e009575f124325)

yuha commented

2020-12-05 19:04:56 +01:00

Author

Member

resolved all threads

yuha commented

2020-12-05 19:11:51 +01:00

Author

Member

added 1 commit

b4f75489 - Use mixin for no_cache in class based views

Compare with previous version

added 1 commit <ul><li>b4f75489 - Use mixin for no_cache in class based views</li></ul> [Compare with previous version](/AlekSIS/official/AlekSIS-Core/-/merge_requests/336/diffs?diff_id=4003&start_sha=ce78324eed4b1a7d8e09430c07370eca581f4831)

nik commented

2020-12-05 19:19:57 +01:00

Owner

Unused import

nik commented

2020-12-05 19:19:57 +01:00

Owner

Apart from the outdated super syntax (just use super()` nowadays), this is a pretty useless mixin.

Apart from the outdated `super` syntax (`just use `super()` nowadays), this is a pretty useless mixin.

nik commented

2020-12-05 19:19:57 +01:00

Owner

Please use method_decorator with the never_cache decorator directly on the class instead.

Please use `method_decorator` with the `never_cache` decorator directly on the class instead.

nik commented

2020-12-05 19:19:58 +01:00

Owner

Please use method_decorator with the never_cache decorator directly on the class instead.

Please use `method_decorator` with the `never_cache` decorator directly on the class instead.

nik commented

2020-12-05 19:19:58 +01:00

Owner

Please use method_decorator with the never_cache decorator directly on the class instead.

Please use `method_decorator` with the `never_cache` decorator directly on the class instead.

nik commented

2020-12-05 19:20:18 +01:00

Owner

Where is this used, and why?

nik commented

2020-12-05 19:21:35 +01:00

Owner

Please clean up the commit history (squashing commits that add/remove the same thing).

yuha commented

2020-12-05 19:21:40 +01:00

Author

Member

Why though?

yuha commented

2020-12-05 19:23:42 +01:00

Author

Member

Nowhere. Forgot to remove it.

nik commented

2020-12-05 20:12:38 +01:00

Owner

From the Zen of Python:

Explicit is better than implicit.
Readability counts.

Using the decorator directly is as short as using the mixin, but clearly conveys what is happening without looking at another file.

There should be one-- and preferably only one --obvious way to do it.

There is already one obvious way to apply a decorator to a class-based view, as described in the Django docs.

Also, if a view decides to override the dispatch method, the mixin would break, requiring developers to mix both ways in the same codebase, and developers might need finer-grained control over which methods are decorated (e.g. decorating options or head is wrong in quite a few cases, requiring to drop the mixin and go with the method decorator again).

From the Zen of Python: ```plain Explicit is better than implicit. Readability counts. ``` Using the decorator directly is as short as using the mixin, but clearly conveys what is happening without looking at another file. ```plain There should be one-- and preferably only one --obvious way to do it. ``` There is already one obvious way to apply a decorator to a class-based view, as described in the Django docs. Also, if a view decides to override the `dispatch` method, the mixin would break, requiring developers to mix both ways in the same codebase, and developers might need finer-grained control over which methods are decorated (e.g. decorating `options` or `head` is wrong in quite a few cases, requiring to drop the mixin and go with the method decorator again).

nik commented

2020-12-05 20:15:27 +01:00

Owner

Also, you should research how method resolution works with multiple inheritance, Your mixin breaks surprisingly easily.

yuha commented

2020-12-06 13:39:27 +01:00

Author

Member

changed this line in version 17 of the diff

changed this line in [version 17 of the diff](/AlekSIS/official/AlekSIS-Core/-/merge_requests/336/diffs?diff_id=4005&start_sha=b4f754893f7553a83cab6e36e3ae2561014b44e3#c5bff67ef2f26802f3c80e9bf77dc3105d8fc8ec_7_7)

yuha commented

2020-12-06 13:39:27 +01:00

Author

Member

changed this line in version 17 of the diff

changed this line in [version 17 of the diff](/AlekSIS/official/AlekSIS-Core/-/merge_requests/336/diffs?diff_id=4005&start_sha=b4f754893f7553a83cab6e36e3ae2561014b44e3#9104f183b254581056a368588770f82082815153_437_430)

yuha commented

2020-12-06 13:39:28 +01:00

Author

Member

changed this line in version 17 of the diff

changed this line in [version 17 of the diff](/AlekSIS/official/AlekSIS-Core/-/merge_requests/336/diffs?diff_id=4005&start_sha=b4f754893f7553a83cab6e36e3ae2561014b44e3#ca5e4a886c594c4b59402dbdbd16348bdbcddf3a_37_38)

yuha commented

2020-12-06 13:39:29 +01:00

Author

Member

changed this line in version 17 of the diff

changed this line in [version 17 of the diff](/AlekSIS/official/AlekSIS-Core/-/merge_requests/336/diffs?diff_id=4005&start_sha=b4f754893f7553a83cab6e36e3ae2561014b44e3#ca5e4a886c594c4b59402dbdbd16348bdbcddf3a_115_116)

yuha commented

2020-12-06 13:39:29 +01:00

Author

Member

changed this line in version 17 of the diff

changed this line in [version 17 of the diff](/AlekSIS/official/AlekSIS-Core/-/merge_requests/336/diffs?diff_id=4005&start_sha=b4f754893f7553a83cab6e36e3ae2561014b44e3#ca5e4a886c594c4b59402dbdbd16348bdbcddf3a_126_128)

yuha commented

2020-12-06 13:39:29 +01:00

Author

Member

changed this line in version 17 of the diff

changed this line in [version 17 of the diff](/AlekSIS/official/AlekSIS-Core/-/merge_requests/336/diffs?diff_id=4005&start_sha=b4f754893f7553a83cab6e36e3ae2561014b44e3#abc9d828b78bd9d36e05475d0bc91119bc5bc6b0_528_527)

yuha commented

2020-12-06 13:39:30 +01:00

Author

Member

added 2 commits

0d92c3bc - Remove unused setting
b77baad4 - Remove unused import

Compare with previous version

added 2 commits <ul><li>0d92c3bc - Remove unused setting</li><li>b77baad4 - Remove unused import</li></ul> [Compare with previous version](/AlekSIS/official/AlekSIS-Core/-/merge_requests/336/diffs?diff_id=4005&start_sha=b4f754893f7553a83cab6e36e3ae2561014b44e3)

yuha commented

2020-12-06 13:43:36 +01:00

Author

Member

resolved all threads

nik commented

2020-12-06 13:46:26 +01:00

Owner

Unrelated change

nik commented

2020-12-06 13:46:27 +01:00

Owner

Please strip blank lines from the end.

yuha commented

2020-12-06 14:00:37 +01:00

Author

Member

changed this line in version 18 of the diff

changed this line in [version 18 of the diff](/AlekSIS/official/AlekSIS-Core/-/merge_requests/336/diffs?diff_id=4006&start_sha=b77baad453b8e2e213c1ef4727ca715f570af873#abc9d828b78bd9d36e05475d0bc91119bc5bc6b0_525_525)

yuha commented

2020-12-06 14:00:38 +01:00

Author

Member

changed this line in version 18 of the diff

changed this line in [version 18 of the diff](/AlekSIS/official/AlekSIS-Core/-/merge_requests/336/diffs?diff_id=4006&start_sha=b77baad453b8e2e213c1ef4727ca715f570af873#e142ec3315fcf067ef796e64b486030b06c6dd2e_96_94)

yuha commented

2020-12-06 14:00:39 +01:00

Author

Member

added 7 commits

9ab8dadb - Read headers in SW to decide whether to cache a page or not (via cache-control)
1d6d6ce9 - Add never_cache decorator for several views
a65f6085 - Remove dynamic serviceworker generation
ef5c5edb - Remove unused setting
b19c4259 - Remove unused import
f2c53de8 - Fixed precaching of offline fallback page
481607a3 - Reformat

Compare with previous version

added 7 commits <ul><li>9ab8dadb - Read headers in SW to decide whether to cache a page or not (via cache-control)</li><li>1d6d6ce9 - Add never_cache decorator for several views</li><li>a65f6085 - Remove dynamic serviceworker generation</li><li>ef5c5edb - Remove unused setting</li><li>b19c4259 - Remove unused import</li><li>f2c53de8 - Fixed precaching of offline fallback page</li><li>481607a3 - Reformat</li></ul> [Compare with previous version](/AlekSIS/official/AlekSIS-Core/-/merge_requests/336/diffs?diff_id=4006&start_sha=b77baad453b8e2e213c1ef4727ca715f570af873)

yuha commented

2020-12-06 14:00:50 +01:00

Author

Member

resolved all threads

nik commented

2020-12-06 14:13:29 +01:00

Owner

Still not clean.

While at it, also please reword all commit messages that fo not follow our commit message guidelines.

Still not clean. While at it, also please reword all commit messages that fo not follow our commit message guidelines.

nik commented

2020-12-06 21:22:33 +01:00

Owner

added 34 commits

481607a3...5daa73f9 - 31 commits from branch master
9c251e1a - Add serviceworker template, settings and file replacement functionality
01d5b7f7 - Add never_cache decorator for several views
d03ac424 - Fix precaching of offline fallback page

Compare with previous version

added 34 commits <ul><li>481607a3...5daa73f9 - 31 commits from branch <code>master</code></li><li>9c251e1a - Add serviceworker template, settings and file replacement functionality</li><li>01d5b7f7 - Add never_cache decorator for several views</li><li>d03ac424 - Fix precaching of offline fallback page</li></ul> [Compare with previous version](/AlekSIS/official/AlekSIS-Core/-/merge_requests/336/diffs?diff_id=4008&start_sha=481607a334744807af62651bda93ac8a85c5f0d0)

yuha commented

2020-12-06 21:24:07 +01:00

Author

Member

resolved all threads