Conversation
reinhardt
force-pushed
the
dsetool-choices
branch
from
c3d5a46 to
e261a1a
Compare
reinhardt
force-pushed
the
dsetool-choices
branch
from
85a76bc to
1350476
Compare
src/euphorie/client/navigation.py
Outdated
| model.Choice.id == model.SurveyTreeItem.id, | ||
| sql.or_( | ||
| model.Choice.condition == None, # noqa: E711 | ||
| model.Choice.condition.like(model.Option.zodb_path), |
There was a problem hiding this comment.
This is unclear to me...
You are using like, but AFAIK like requires a pattern to match.
Is model.Choice.condition.like(model.Option.zodb_path) containing an %?
If not, it seems to me that this is equivalent to:
model.Choice.condition == model.Option.zodb_path
If yes, it would be unexpected for me.
ale-rt
left a comment
ale-rt
left a comment
There was a problem hiding this comment.
I did a first round of review on the code.
Disclaimer:
- I do not understand much what is going
- I never see the feature live
- I just have a vague idea about what this feature should be
| def my_context(self): | ||
| return aq_inner(self.context) |
There was a problem hiding this comment.
I know this is copied from existing code, but I think this is redundant.
| "url": option.absolute_url(), | ||
| "title": option.title, | ||
| } | ||
| for option in self.my_context.values() |
There was a problem hiding this comment.
What benefit is giving us to decrease the amount of information we have by converting a full object into a dict?
It seems to me that returning the option instances instead of the dicts would be better.
The advantages would be that you have more information and less code and indirection.
There was a problem hiding this comment.
Consistency, I guess. It's done the same way with the solutions in the risk view.
| <object meta_type="Conditional Dexterity FTI" | ||
| name="euphorie.choice" | ||
| /> | ||
| <object meta_type="Conditional Dexterity FTI" |
There was a problem hiding this comment.
Does this need to be a Conditional Dexterity FTI?
Or is an only addable in choices?
There was a problem hiding this comment.
I guess it could also be a regular FTI. Currently there is no restriction.
| @@ -0,0 +1,75 @@ | |||
| <?xml version="1.0" encoding="utf-8"?> | |||
| <object xmlns:i18n="http://xml.zope.org/namespaces/i18n" | |||
| meta_type="Dexterity FTI" | |||
There was a problem hiding this comment.
Should the meta_type be adapted as well?
There was a problem hiding this comment.
For the module type there is the same discrepancy:
But I suppose it would make more sense if they matched, yes.
| ) | ||
| type = schema.Column( | ||
| Enum(["risk", "module"]), | ||
| Enum(["risk", "choice", "module"]), |
There was a problem hiding this comment.
I think that modifying an Enum requires an alembic upgrade, but I might be wrong.
There was a problem hiding this comment.
I used autogenerate to create the alembic upgrade and it didn't contain anything about the Enum. Also, it works fine as it is.
There was a problem hiding this comment.
Maybe you initialized the DB after the change in Python code was done?
Might I ask you to double check this?
This might help:
If after that's not the case and an upgrade is not needed, you can resolve this conversation.
There was a problem hiding this comment.
I haven't initialized this DB in years.
The type column is just a varchar, there's nothing to upgrade there.
type | character varying(6) | | not null |
|
Sorry, found another issue. Small fix incoming ... |
|
OK, ready for another round of review. |
| if self.webhelpers.redirectOnSurveyUpdate(): | ||
| return |
There was a problem hiding this comment.
I do not understand this if clause.
It seems to me the code is equivalent to:
| if self.webhelpers.redirectOnSurveyUpdate(): | |
| return | |
| self.webhelpers.redirectOnSurveyUpdate(): |
To me
There was a problem hiding this comment.
Copied from
Euphorie/src/euphorie/client/browser/risk.py
Line 627 in 781195c
My guess is that it's to make sure that any code that might be added after this line is not executed if the redirect kicks in.
| _next = reply.get("next", None) | ||
| # In Safari browser we get a list | ||
| if isinstance(_next, list): | ||
| _next = _next.pop() |
There was a problem hiding this comment.
I like more what you did on the other view, where you have a dedicated _get_next method.
Any reason to not do the same here?
There was a problem hiding this comment.
The goal right now is only to move this from browser/risk.py so that we can use it for choices, too. Refactoring can be done later.
| return self.request.response.redirect( | ||
| "{session_url}/@@start".format( | ||
| session_url=self.webhelpers.traversed_session.absolute_url() | ||
| ) | ||
| ) |
There was a problem hiding this comment.
To make the code more readable I would add a def redirect method that would make this possible:
| return self.request.response.redirect( | |
| "{session_url}/@@start".format( | |
| session_url=self.webhelpers.traversed_session.absolute_url() | |
| ) | |
| ) | |
| return self.redirect("@@start") |
or something similar, IDK.... self.webhelpers.redirect_to_session_path or whatever.
That can be useful also for the other view.
There was a problem hiding this comment.
Yes, but let's refactor later and also include browser/risk.py to keep things halfways consistent.
| .first() | ||
| ) | ||
| if not sql_module: | ||
| url = self.context.absolute_url() + "/@@identification" |
There was a problem hiding this comment.
I am big fan of fstring:
| url = self.context.absolute_url() + "/@@identification" | |
| url = f"{self.context.absolute_url()}/@@identification" |
They should be even faster, but what you did has nothing wrong.
| def is_visible(self): | ||
| if not self.condition: | ||
| return True | ||
| options = self.condition.split("|") |
There was a problem hiding this comment.
Apparently it is visible only if we have some paths.
What about calling this column allowed_paths?
There was a problem hiding this comment.
I don't understand. Which column? Why rename it?
There was a problem hiding this comment.
I don't understand. Which column? Why rename it?
The column condition. It seems that allowed_paths is a better name that will help us understand what's going on
There was a problem hiding this comment.
No, sorry, that doesn't fit at all. It's a condition. If any of the options that correspond to the paths is picked, then the choice is visible.
| return False | ||
| return True | ||
|
|
||
| def set_options_by_zodb_path(self, paths): |
There was a problem hiding this comment.
Skimming the code it seems to me that using sets might make the code more readable. Please have a thought about that
There was a problem hiding this comment.
I had a hard to getting this to run without errors, I'm reluctant to touch it again.
There was a problem hiding this comment.
I see your point, would the code simplify if instead of having a relation to an Option we would just store the options as a | concatenated string?
Then the option table could just go away.
There was a problem hiding this comment.
Then we can't do joins properly.
There was a problem hiding this comment.
Are you making any joins in this PR.
AFAICS the code sort of enforces a 1 to 1 relationship between choices and options.
I might get the code wrong by just reading it.
Let's discuss this in a call.
Co-authored-by: Alessandro Pisa <alessandro.pisa@gmail.com>
| <value_type type="plone.registry.field.TextLine" /> | ||
| <default>{ | ||
| "tiles": "\nnavtree [context.portal_type in ['euphorie.profilequestion', 'euphorie.module', 'euphorie.risk', 'euphorie.solution', 'euphorie.survey', 'euphorie.surveygroup', 'euphorie.folder', 'euphorie.documentation', 'euphorie.help', 'euphorie.page', 'euphorie.training_question', 'euphorie.choice', 'euphorie.option'] ]\neuphorie.usermgmt.navtree [context.portal_type=='euphorie.country' and request.getURL().endswith('@@manage-users')]", | ||
| "type": "group" |
There was a problem hiding this comment.
Generally speaking, I would not rewrite the record but modify it.
You decrease the risk that something modified by hand gets rewritten
There was a problem hiding this comment.
It's not possible to modify this by hand. Try it.
2 participants
Ref https://github.com/syslabcom/scrum/issues/3326