Skip to content
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

Publish subscribe ajax #183

Closed

Conversation

JuliusR
Copy link
Contributor

@JuliusR JuliusR commented Oct 3, 2013

The problem: wrong controller

StockArticle manipulation in Delivery form

The changes in #141 made it easier to create a delivery because the user can create and update StockArticles in the delivery form. However, a probably bad practice was used: All the StockArticle manipulation was done in the Deliveries controller. Here I would like to suggest a new pattern in order to

  • move the StockArticle manipulation to the correct controller
  • provide an easy way to perform the DOM updates in the current view (Deliveries, StockArticles, ...).

More use cases

I am planning to provide similar features for StockArticles in the StockTaking form, so there is the need of at least one more application of this pattern in the future.

How it could work (my suggestion)

Let us assume the current view is views/deliveries/new.html.haml and we would like to create a new StockArticle.

  1. AJAX request (GET) to StockArticle#new
    • open a modal by views/stockit/new.js.erb
  2. AJAX request (POST) to StockArticle#create
    • display a universal success message by views/stockit/create.js.erb and do $('body').trigger('StockArticle#create', ...) passing the new StockArticle's id
  3. the .trigger(...) causes another AJAX request (GET) to Delivery#on_stock_article_create
    • handle the DOM update by views/deliveries/on_stock_article_create.js.erb

Disadvantages of this approach

  • Readability: Is it clear what is going on?
  • Two subsequent AJAX requests for one creation/update
  • Still some redundancy in those controllers which use the function in their views (see actions named ..._on_stock_article_create, ..._on_stock_article_update etc.)

Question to you

Would you be fine if I followed this approach to the end? Do you have some experience in that and/or a better idea?

@wvengen
Copy link
Member

wvengen commented Oct 31, 2013

Hi, sorry for taking so long to review. I think it's a good step towards clearer separation - only those _on_ methods bother me a bit, but it's ok. Some documentation would be beneficial, I think.

Nice idea to use javascript triggers.

@JuliusR
Copy link
Contributor Author

JuliusR commented Oct 31, 2013

Thanks for your comments.

How do you think about the comments I added in the source code?

The idea with the javascript trigger (as well as the basic idea) I learned from a book about javascript design patterns.

@wvengen
Copy link
Member

wvengen commented Oct 31, 2013

Ah, that's clearer, thanks! I'm doubting which would be better: duplicating the comments in the source vs. a separate document (in doc/ ?) explaining the whole process, referenced from the source with a pointer to where in the process it is, and mentioning exceptions.

p.s. nice book!

@JuliusR
Copy link
Contributor Author

JuliusR commented Nov 1, 2013

a separate document (in doc/ ?) explaining the whole process, referenced from the source with a pointer to where in the process it is, and mentioning exceptions

Nice idea. I just considered explaining the stuff once in the source code and then referencing. But my idea I rejected due to bad maintainability (if the single view with the comment was modified/renamed/..., all references to that place might have needed an update)

@wvengen
Copy link
Member

wvengen commented Nov 13, 2013

Something similar would be useful for balancing too. Using it in the beginnings of a new receive screen (and I'd like to be able to create a new order article on the spot - very much like deliveries).

@JuliusR
Copy link
Contributor Author

JuliusR commented Nov 30, 2013

My aim for next week is to implement all suggestions so far here.

Sorry I had a busy time and could not follow all discussions in other issues etc. If you think there is anything more urgent, please let me know.

@JuliusR
Copy link
Contributor Author

JuliusR commented Dec 7, 2013

Closed in favor of #219.

@JuliusR JuliusR closed this Dec 7, 2013
@JuliusR JuliusR deleted the publish-subscribe-ajax branch December 21, 2013 21:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants