-
Notifications
You must be signed in to change notification settings - Fork 16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
administration: integrate banners view module #15
administration: integrate banners view module #15
Conversation
b6a5166
to
d248ea9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Added a couple of comments
"description": _( | ||
"Message to be displayed on the banner. HTML format is supported." | ||
), | ||
"rows": 10, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you might want to add to this PR the bump of the min version of react-invenio-forms
in the webpack.py
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It has to be done in invenio-administration
PR inveniosoftware/invenio-administration#154
After the react-invenio-forms
is released
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job!
Left some minor comments
* integrate admin banners list, edit, create and details view * disable expired banners after each create and update action * improve banners marshmallow schema * add http request data validation at create and update actions * improve banners macro * override banners list result to_dict() * move getting active banners to only db layer * cover macros with tests * closes inveniosoftware#10
d248ea9
to
03882d8
Compare
def test_disable_expired_after_create_action(client, admin, headers): | ||
"""Disable expired banners after a create a banner action.""" | ||
# create banner first | ||
banner1 = BannerModel.create(banners["banner1"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about this but maybe as we are testing the resource we shouldn't use the model directly?
def read_active_banners(self, identity, url_path): | ||
"""Retrieve the list of active banners with the given url_path.""" | ||
self.require_permission(identity, "read") | ||
|
||
active_banners = self.record_cls.get_active() | ||
|
||
# filter by url_path | ||
banners = [] | ||
for banner in active_banners: | ||
if banner.url_path is None or url_path.startswith(banner.url_path): | ||
banners.append(banner) | ||
|
||
return self.result_list( | ||
self, | ||
identity, | ||
banners, | ||
links_item_tpl=self.links_item_tpl, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just by curiosity. Why removing this function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved the filtering to the db layer, because it's easier to access from utils get_active_banners_for_request
(I had some issues with accessing service methods from there).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks for the tests!
Search view page (with pagination):


Details view:


Create banner view:

Create banner action:

Edit banner view:

Edit banner action:

Delete banner action:

Sort banners action:

By Url path
By Active

By Start Datetime

Search banners action:

Search by string field
Search by datetime field

Search by bool field

Loader preview:

Plain HTML into banner message:
