Delete organigram in model before exchanging

[Tschuppi81]

Agency: Fixes required forced browser refresh (Ctrl+F5) after organigram replacement

TYPE: Bugfix
LINK: ogc-1346

[codecov-commenter]

Codecov Report

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

Comparison is base (a914aa7) 87.64% compared to head (8135f42) 87.63%.
Report is 23 commits behind head on master.

Files	Patch %	Lines
src/onegov/file/integration.py	93.33%	1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1090      +/-   ##
==========================================
- Coverage   87.64%   87.63%   -0.02%     
==========================================
  Files        1185     1185              
  Lines       77924    77939      +15     
==========================================
  Hits        68300    68300              
- Misses       9624     9639      +15     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Tschuppi81]

@Daverball I found when exchanging the organigram attachement I need to delete the model.organigram to ensure a new storage id will be created with the new attachement and therefore a browser refresh is not needed.
Is this the right approach?

[Daverball]

You can probably use Cache-Control headers instead, we use them heavily in ElectionDay views, since those are cached. You may need to add a separate view for the HEAD request which only sets the header.

[Daverball]

Not sure if disabling the cache globally is the best idea, I was more thinking of setting headers in individual views. Although if it's just the standard file view (i.e. the organigram doesn't use its own view) the flaw might be in onegov.file we should probably send the timestamp of the last modification on the file so the browser can decide whether the cache needs to be invalidated or not.

[Tschuppi81]

I found Events do not have the issue when replacing the image. So l looked into it. The difference is form.populate_obj() vs form.update_model(). I will investigate on this...

[Daverball]

populate_obj is just the regular wtforms method that does a setattr on the object for each field in the form, update_model is a convention we sometimes use when the form doesn't map as cleanly and we have to write most of the attribute setting ourselves anyways, so that shouldn't really matter, from what I can tell the organigram is a regular file, so the issue is just that we don't send a last modified header, so the browser has no idea when the file is out of date and needs to be refreshed, hence the need for manually refreshing.

You can delete the file and generate a new one so the link changes, but setting the last modified header seems like a more permanent solution, since this affects all uploaded files and not just this one. See here for how we set the last modified header in election day:

onegov-cloud/src/onegov/election_day/utils/common.py

Lines 59 to 69 in 1fb9395

	def add_last_modified_header(
	response: 'Response',
	last_modified: 'datetime | None'
	) -> None:
	""" Adds the give date to the response as Last-Modified header. """
	if last_modified:
	response.headers.add(
	'Last-Modified',
	last_modified.strftime("%a, %d %b %Y %H:%M:%S GMT")
	)

You should be able to retrieve the last modified from either the file reference, or you can use the timestamp on the file model which should also change when the reference changes. The file view is here:

onegov-cloud/src/onegov/file/integration.py

Lines 486 to 490 in 1fb9395

	def view_file(self: File, request: 'CoreRequest') -> 'StoredFile':
	respond_with_alt_text(self, request)
	respond_with_caching_header(self, request)
	respond_with_x_robots_tag_header(self, request)
	return self.reference.file

It might be worth adding a HEAD view for files that just sets the headers. Here's an example of a head view in election day:

onegov-cloud/src/onegov/election_day/views/election/main.py

Lines 28 to 42 in 1fb9395

	@ElectionDayApp.view(
	model=Election,
	request_method='HEAD',
	permission=Public
	)
	def view_election_head(
	self: Election,
	request: 'ElectionDayRequest'
	) -> None:
	@request.after
	def add_headers(response: 'Response') -> None:
	add_cors_header(response)
	add_last_modified_header(response, self.last_modified)

[Tschuppi81]

Unfortunately the 'last-modified' response header did not resolve the issue although it was set correctly. A regular refresh did not reload the image resource, see last commit.

The root cause, in my opinion, is the resource (organigram) does not get a new id when exchanging the image. if i delete first, then add a new organigram I get a new resource id hence a proper page reload.

I suggest to go back to my original attempt which ends up in a new resource id upon exchanging the organigram.
What do you think @Daverball?

[Daverball]

You need to set it in the file view not the view that links to the file, the file is a separate request, setting it on the html does absolutely nothing, since that isn't cached to begin with (unless you manually enable it through cache control headers), that's why i pointed you towards the file view and not the agency view.

This is also why I said it affects all files and not just the ones in agency. So this issue will crop up again and again unless we fix it correctly.

[Tschuppi81]

After moving the add_last_modified_headers to the 'file view head' and some debugging I see that view_file is only called on browser 'force refresh' but not on 'refresh'. On the other hand view_file_head doesn't get called in either case. I might still doing wrong or the approach does not work out.

I also found that the def organigram_file setter method handles two cases, one to replace one for a new organigram file.
A very similar use case to this can be found in the Agency model. There we have a pdf_file setter method doing the same with a pdf file. Instead it always does a new pdf, hence a new id. So I tried the same in organigram_file (always doing the 'else' path) and it worked.
As you said the downside is that it only solved the issue for agency...

[Daverball]

@Tschuppi81 It might have something to do with that we're also setting max age on the Cache-Control header. Caching behavior is weird and complicated. Do you still not see any HEAD requests if you remove the max age? I think we're throttling the HEAD requests on files to one every minute, so it would take at least one minute to see changes reflected on files.

It's also possible that bjoern does some file caching of its own, to speed up file requests, of which there are usually many. In which case I guess we can rely on the workaround.

[Daverball]

Also you might need to add the last modified header to be present in the file view as well, so the browser has a frame of reference.

Since the head view calls the file view, you should just be able to move the function that sets the last modified header to the file view.

[Tschuppi81]

No, I still don't see any head views calls when removing max age...

I also did various changes regarding caching, changing the headers. Nothing helped there as the file view does not get called for after editing the organigram of an Agency. In contrast changing the image of an Event always invokes the file view leading to a proper refreshed view.

So I started to investigate on the different behavior and found the Agency model changes the reference under the hood and the organigram does not get to know it. So it is better to set a new organigram like show below.

After changing the Agency model, organigram_file setter from

        if self.organigram:
            self.organigram.reference = as_fileintent(value, filename)
            ...
        else:
            organigram = AgencyOrganigram(id=random_token())
            ...

to

        organigram = AgencyOrganigram(id=random_token())
        organigram.reference = as_fileintent(value, filename)
        organigram.name = filename
        self.organigram = organigram

it starts working, independent of caching changes.

[Daverball]

I'd like to investigate this myself at some point, but I guess there's also the issue of File deriving a whole bunch of meta information from StoredFile when it is first created. I forget if associated with a one-to-one cascades the delete, I think it does, but it's probably a good idea to verify in a test that the old file gets deleted.

I don't mind changing the behavior to replace the file entirely in this specific case, although I'd do a git blame on it to investigate whether this branch has been added intentionally to fix another bug, or if maybe the branch used to be there in the other model and was removed for the same reason.