int(request.REQUEST.get('index', 0) all in try/except. See #9918

[will-moore]

Can't assume that request strings can be cast to int().

9918 bug is due to adding of 'index' request parameter to various edit actions of the right hand panel, to cater for Well editing (which actually edits image at specified index).

This, combined with c35bac9 causes the bug. The single commit on this branch effectively reverts c35bac9, wrapping int(index) in a try / except.

A more thorough fix will be to remove the 'index' logic from the right-hand panel where not needed.

[joshmoore]

Does anyone know if this is intended for 4.4.5?

[jburel]

@joshmoore; I will not take the chance, so I will exclude it.

[will-moore]

Depends whether #9918 is a blocker (can't edit Tag name / description in web).
The commit "shouldn't" cause any problems, since it's only adding try/except statements, but still, if #9918 isn't a blocker then no need to risk it.

[joshmoore]

@will-more, Chris also had some questions about the strategy in general, so it'll need to be omitted for 4.4.5.

[will-moore]

@chris-allan You have some questions about the "strategy in general"? Can you summarise here, or do you want to discuss?

[chris-allan]

Given that we are now after 4.4.5 my position is that we should be a little more thorough with this fix. Specifically I'd like to see the removal of the unneeded parsing from load_data_by_tag() and the switch to a more specific and documented try block such as:

# the index of a field within a well, can be given to us as an empty string
try:
    index = int(request.REQUEST.get('index', 0))
except ValueError:
    index = 0

I admit to not knowing the consequences on the code if index < 0. Perhaps a method should be used to be more thorough and consistent about parsing and error checking this variable?

It does also seem needless to be attempting this parsing action when executing a view function that does not target a Plate or Well. Perhaps we should be doing it only where required? Furthermore, does an error condition when a Plate or Well is being targeted make more sense rather than blindly using index=0?

[will-moore]

Hmmm - OK, just need to think how to do this without some horribly long template if/else statements!

[joshmoore]

Just an idea: is there any way to extend request/REQUEST to add a method getIntOrDefault('index', 0) ? Failing that, something on self like getInt(request, 'index', 0).

[will-moore]

@joshmoore @chris-allan Can I get this back into a build now? (remove the 'needs-work' label)?

[jburel]

@will-moore: done

[joshmoore]

print should go.

[joshmoore]

Is there any way that {{ index }} can contain something that won't be appropriately appendable to ?index=? That's the only thing I can think to check, but otherwise this is good to go.

[will-moore]

@joshmoore {{ index }} should only ever contain a valid integer. Even if you manually edited a url with ?index=invalid then this would be caught by the try except that was added above.

[chris-allan]

All seems a bit more thorough now. Merging.