Minor Homepage enhancements for Nepal #1084

michaelhowden · 2015-05-02T02:14:16Z

No description provided.

flavour · 2015-05-07T18:26:03Z

controllers/inv.py

@@ -1009,6 +1009,11 @@ def set_track_attr(status):
            tracktable.recv_bin.writable = True

    def prep(r):
+        if r.vars.get("recv.status") == '2':
+            s3.crud_strings.inv_recv.title_list = T("Existing Shipments to Received")


"Existing Shipments to Receive"?

I am wary of making this change as it means new strings to translate suddenly.
Especially wary just after Honduras Red Cross spent a long time going through this module & tweaking...perhaps this could go into template?

Although I'm wary about any one template dictating default... Moved to the template for now - but I think that these changes could have generally improved UX with inv

nursix · 2015-05-07T18:48:03Z

I don't see why we need the SCSS of font-awesome in trunk? We're not going to customize it, are we?
I think the CSS is absolutely good enough

flavour · 2015-05-07T18:48:14Z

static/fonts/font-awesome/less/animated.less

@@ -0,0 +1,34 @@
+// Animated Icons


We shouldn't really have all this scss in the fonts folder I don't think

michaelhowden · 2015-05-08T03:42:11Z

@nursix + @flavour Just to clarify, I was under no assumptions that this code was trunk worthy - hence no PR. However your review was extremely timely. I've tried to address all of these - with the last outstanding this being the deprecation of FA3

michaelhowden · 2015-05-08T04:15:11Z

PR Updated

nursix · 2015-05-08T05:46:13Z

Still a lot of .less files without need. LESS is even /less/ needed than SCSS ;)

CSS and font files should be good enough.

nursix · 2015-05-08T05:55:31Z

modules/s3/s3query.py

@@ -676,7 +676,8 @@ def __init__(self, resource, selector, label=None):
        # Fall back to the field label
        if label is None:
            fname = self.fname
-            if fname in ["L1", "L2", "L3", "L3", "L4", "L5"]:
+            # Does this really belong here? Better on gis_location fields 


This does belong here! The labels vary by config location, so the model must be loaded before you can determine which labels to use. Please remove the comment.

nursix · 2015-05-08T10:53:22Z

Interesting that you say there was no PR - obviously this here is a PR?

michaelhowden · 2015-05-26T06:17:51Z

@nursix Changes made - how's this look?

nursix · 2015-05-29T22:41:32Z

controllers/default.py

@@ -63,8 +63,13 @@ def register_validation(form):

 # =============================================================================
 def index():
-    """ Main Home Page """
-
+    """ Customized Main Home Page (in modules/<TEMPLATE>/config.py) """


I'm -1 to introducing this - we already have a (much better) method to customize the home page controller, and this introduces double standards, which is confusing.

But I can see why you did this - you wanted to have access to the IM layout definition.

However, that's not necessary - templates are now just normal modules, so you can move everything into the right place:

move the IM layout into layouts.py

move the inv index controller into controllers.py

...and import:

import IM into controllers.py like: from layouts import IM

import the inv index controller into config.py like: from controllers import customise_inv_homepage

Alternatively, you can keep the customise_inv_homepage in config.py, and import the IM layout there.

You can even import across templates, e.g. a layout like: from templates.Nepal.layouts import IM

...so there is less need to duplicate code across templates.

Hmmm... (pondering)
I was expecting some debate around this - and generally I'm OK with this approach!
Some comments:

One main reason to move it into config.py is to avoid restarting web2py for every change!

We already have double standards with the "customise_home" setting... should that have been introduced?

I actually see a strong reason for putting more in controllers.py than config.py so that it can be reused between templates - eg, let's say that the customise_org_organization_controller needed to be reused in multiple templates... is this right? (Not something I'm planning to do right away)

Thanks for the import pointers - took <5min to change :)

måndagen den 1 juni 2015 14.50.34 skrev Michael Howden:

@@ -63,8 +63,13 @@ def register_validation(form):

========================================================================
=====>
def index():

- """ Main Home Page """

""" Customized Main Home Page (in modules//config.py) """
Hmmm... (pondering)
I was expecting some debate around this - and generally I'm OK with this
approach! Some comments:

One main reason to move it into config.py is to avoid restarting web2py
for every change!
This is out-of-date information.

All of the template is now imported, so it doesn't make any difference whether
it's config.py or controllers.py. As long as you have module tracking enabled
(debug mode), there's no need to restart web2py.

However, note that git repository dates may be older than your .pyc's so that
applying changes with git (e.g. switching branches) may however require a
web2py restart.

Also, Eclipse does sometimes not follow these re-imports - even if web2py does
(that's hearsay, though - I'm not using Eclipse).

Generally, one should be more relaxed about having to restart web2py - if run
from the command line, it's just two key strokes away, and not really
disruptive for the flow. We can not seriously make code design decisions
dependend on such.

nursix · 2015-05-29T23:22:45Z

Sorry for the delay with the review, couldn't find the time earlier. Please note that there are merge conflicts, so rebasing may be necessary.

michaelhowden · 2015-06-01T22:54:21Z

@nursix Thanks for the review. Everything addressed and merge conflicts resolved (nothing major) - hopefully good to merge!

nursix · 2015-06-02T05:47:03Z

modules/s3/s3codecs/xls.py

@@ -171,6 +171,9 @@ def encode(self, data_source, **attr):
        list_fields = attr.get("list_fields")
        if not list_fields:
            list_fields = data_source.list_fields()
+        # Always export comments field
+        if "comments" in data_source.fields and "comments" not in list_fields:


Clear -1 to this - this should definitely not be enforced in the codec, and especially not without any option to turn it off. Exporting comments is not a general requirement for every deployment, and not for every resource either.

nursix · 2015-06-02T06:32:12Z

There's a lot of non-standard code formatting here still, like blank lines missing where they should be, inconsistent identation (closing brackets), non-standard line breaks, extra whitespace ... If Fran wants to clean it up after merge, then fine - but I actually think you should do that yourself.

Enforcing the comments field in the XLS codec is plain wrong - especially without any option to turn it off. And neither can I see why you would remove some of the settings from IFRC/config.py.

Regarding the homepage customization patterns - there is a significant difference between the default/index customization and the module index pages: the default/index pattern allows for a multi-page extension, which is very useful for e.g. embedding online documentation. This pattern is by far more powerful and cleaner in view of error handling and forwarding than the customise_xxx_homepage pattern.

I am not sure whether the customise_xxx_homepage pattern should have been introduced - to me it always looked like a hack, and not very thought-through. The idea seems ok, but the design is questionable. Especially it does not take into account that errors redirect to the module index (and sometimes even swallows the errors), nor does it consider the implications on permissions. It is also loading functions without need - which is certainly the ugliest part of it (whereas the default/index pattern only ever loads the functions when you actually access default/index). I see this as a rather weak pattern that needs a lot more thought before being applied everywhere.

Regarding the re-usability of code across templates: you are /not/ limited to controllers.py in any way. As I said: templates are now regular Python packages, you can have as many modules inside it as you like. You can import from templates.XYZ.config just as much as from templates.XYZ.controllers or templates.XYZ.layouts.

However, there are of course "moral" limits - some things don't make sense. For example, importing from a template into core would be stupid, so I would be strict about not allowing that. Nor would I actually want things to get imported from one config.py into another (because that'd pave the road for regression bugs).

Generally, we should not strive after maximising cross-template code sharing - if a certain function or class is relevant for many templates, then it should be core. However, there are sometimes two templates within the same general use-case (e.g. RMS and RMS Americas) - and even if they did share certain elements, these elements would still be specific to the case and not necessarily generic.

For such cases (derivates and alternatives), it make maintenance a lot easier to import from one template to the other, and helps to maintain consistency.

However, I would suggest to move this debate to a more visible forum - it's a bit misplaced on your PR.

Note that your PR may take forever if you keep introducing new things into every review iteration - better you just fix/cleanup what you already have in it, and leave the next changes to the next PR. Also note that the more different/independent changes you put into the same PR, the harder it will become to get any of them merged (one-change-blocking-the-other effect).

Inv * Fix Beneficiary Column on Release Report (=To not From) * Distribution Report * Locations in Filters * Add Staff to Nepal Menu * Use clusters General * If the site is only configed for one country, use the hierarchy for that country * Setting for get_supply_autocomplete * Fix in creating hrm * Fix bug in Inv_send filter * Menu implemented usng S3NavigationItem * Font Awesome 4 added * Remove duplicate "Resource Inventory" menu item * "Resource Inventory" -> "Resources" (KISS) * Fix error in vol * Always add comments field to Excel output * Increase org_organisation.acronym.length to import SBTF Data ("USAID, OFDA, US DoD" = 19)

michaelhowden · 2015-06-14T18:14:55Z

I've cleaned things up and addressed all comments. If you let me know where there are specific formatting issues I'll address these too.

michaelhowden · 2015-07-02T02:58:39Z

@flavour Any delay in merging this?

michaelhowden force-pushed the nepal branch 4 times, most recently from 5abb7d3 to f6496a8 Compare May 7, 2015 04:08

flavour reviewed May 7, 2015
View reviewed changes

michaelhowden force-pushed the nepal branch from f6496a8 to 8c87ce0 Compare May 8, 2015 04:11

nursix reviewed May 8, 2015
View reviewed changes

michaelhowden force-pushed the nepal branch 2 times, most recently from 066ed7e to 28136b0 Compare May 26, 2015 06:16

michaelhowden force-pushed the nepal branch from 28136b0 to f5500c4 Compare May 26, 2015 07:06

nursix reviewed May 29, 2015
View reviewed changes

michaelhowden force-pushed the nepal branch from f5500c4 to 85447b9 Compare June 1, 2015 22:54

michaelhowden force-pushed the nepal branch 2 times, most recently from 281c723 to 8a28bad Compare June 2, 2015 04:55

nursix reviewed Jun 2, 2015
View reviewed changes

michaelhowden force-pushed the nepal branch 2 times, most recently from 07a70b7 to ac6448e Compare June 14, 2015 18:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Minor Homepage enhancements for Nepal #1084

Minor Homepage enhancements for Nepal #1084

michaelhowden commented May 2, 2015

flavour May 7, 2015

michaelhowden May 7, 2015

michaelhowden May 8, 2015

nursix commented May 7, 2015

flavour May 7, 2015

michaelhowden May 7, 2015

michaelhowden commented May 8, 2015

michaelhowden commented May 8, 2015

nursix commented May 8, 2015

nursix May 8, 2015

nursix commented May 8, 2015

michaelhowden commented May 26, 2015

nursix May 29, 2015

michaelhowden Jun 1, 2015

michaelhowden Jun 1, 2015

nursix Jun 2, 2015

nursix commented May 29, 2015

michaelhowden commented Jun 1, 2015

nursix Jun 2, 2015

nursix commented Jun 2, 2015

michaelhowden commented Jun 14, 2015

michaelhowden commented Jul 2, 2015

Minor Homepage enhancements for Nepal #1084

Are you sure you want to change the base?

Minor Homepage enhancements for Nepal #1084

Conversation

michaelhowden commented May 2, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nursix commented May 7, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelhowden commented May 8, 2015

michaelhowden commented May 8, 2015

nursix commented May 8, 2015

Choose a reason for hiding this comment

nursix commented May 8, 2015

michaelhowden commented May 26, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

- """ Main Home Page """

nursix commented May 29, 2015

michaelhowden commented Jun 1, 2015

Choose a reason for hiding this comment

nursix commented Jun 2, 2015

michaelhowden commented Jun 14, 2015

michaelhowden commented Jul 2, 2015