Skip to content

[14.0][FIX] dbs value has to be kept in case of special dbfilter config - #2353

Merged
OCA-git-bot merged 1 commit into
OCA:14.0from
invitu:14.0-fix_dbfilter_from_header
Feb 5, 2024
Merged

[14.0][FIX] dbs value has to be kept in case of special dbfilter config#2353
OCA-git-bot merged 1 commit into
OCA:14.0from
invitu:14.0-fix_dbfilter_from_header

Conversation

@cvinh

@cvinh cvinh commented Jun 4, 2022

Copy link
Copy Markdown

I had to commit this fix to make it work on our architecture
Our dbfilter is dbfilter = %d$
Our proxy is Apache 2.4.6

@cvinh
cvinh force-pushed the 14.0-fix_dbfilter_from_header branch from 94240a1 to 526b337 Compare June 4, 2022 22:30
@cvinh cvinh changed the title [FIX] dbs value has to be kept in case of special proxy config [FIX] dbs value has to be kept in case of special dbfilter config Jun 4, 2022
@cvinh cvinh changed the title [FIX] dbs value has to be kept in case of special dbfilter config [14.0][FIX] dbs value has to be kept in case of special dbfilter config Jun 4, 2022
if db_filter_hdr:
dbs = [db for db in dbs if re.match(db_filter_hdr, db)]
else:
dbs = dbs_orig

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you explain what this actually changes, because I don't see it.

dbs = something
if condition:
    dbs = something_other
return dbs

Should be functionally equivalent to:

dbs_orig = something
if condition:
    dbs = something_other
else:
    dbs = dbs_orig
return dbs

@cvinh cvinh Aug 1, 2022

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

thanks for reviewing
In the previous code, as dbs was already in the arguments, it was overwritten by
dbs = db_filter_org(dbs, httprequest)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ahhh i see it now. Yes

@cvinh

cvinh commented Aug 1, 2022

Copy link
Copy Markdown
Author

ping @kh-badep

@thomaspaulb

Copy link
Copy Markdown
Contributor

@hbrunn Could you perhaps glance over quickly and approve also? It's a good fix IMO

@hbrunn hbrunn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

if using the default filter mechanism, why have this module installed in the first place? But I also don't see how this hurts

@hbrunn

hbrunn commented Feb 5, 2024

Copy link
Copy Markdown
Member

/ocabot merge patch

@OCA-git-bot

Copy link
Copy Markdown
Contributor

What a great day to merge this nice PR. Let's do it!
Prepared branch 14.0-ocabot-merge-pr-2353-by-hbrunn-bump-patch, awaiting test results.

@OCA-git-bot
OCA-git-bot merged commit c3a97ac into OCA:14.0 Feb 5, 2024
@OCA-git-bot

Copy link
Copy Markdown
Contributor

Congratulations, your PR was merged at 5537b42. Thanks a lot for contributing to OCA. ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants