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

Enable detail view of reservations #97

Merged
merged 4 commits into from
Dec 17, 2021

Conversation

jat255
Copy link
Member

@jat255 jat255 commented Dec 16, 2021

Resolves #96


Todo:

  • modify group table display to use data_input.items|get_item:key (requiring {% load custom_tags_and_filters %}
    • Still loop the first time to get the key names; rather than second loop, loop through the keys with get_item to ensure they're in the same order as the first loop
  • Address comments below from @rptmat57

Some remaining questions:

  • modal response from failed callback of changing project doesn't show up until modal is closed (only one modal at a time allowed...)
  • the modal-dialog class is set in the base template, and so that has to be changed in templates/base.html to make all modals modal-lg (unless we could make this a parameter somehow)

@rptmat57 I'm sure there's a million things I could have done better, but this seems to be working for me. I'm more than happy to take any criticism or modifications. I think I got the title/project change and cancellation working too, although I may have hosed things up there, so I'd be happy for a review.

I'm not sure if there's any testing or anything that needs to be done. I didn't see formal tests defined in the project, but I may have missed something.

Here's what it looks like as staff or the user owning the reservation:
image

And as a different user:
image

@jat255
Copy link
Member Author

jat255 commented Dec 16, 2021

FYI, the only changes I made that could interact/interfere with other parts of the code were in change_reservation_project of the calendar.py view. I noticed that regardless of what happens when you try to change the project, it was returning a 200, so it looked like it succeeded. I just added some condition checks to return various 40X codes if the project could not be changed.

@rptmat57
Copy link
Contributor

That was quick!
I really like the look of your reservation details table!

Now for my (very detailed) feedback:

  1. We use a limit of 120 characters in .py files NEMO, if you could change that, that'd be great. see https://github.com/usnistgov/NEMO/wiki/Contributions (also run the tests just in case)
  2. I would really prefer not to duplicate the reservation details page. Otherwise every time we make a change in the future, we'll have to remember to change it in 2 separate places. That being said, it makes things a bit tricky with the popup and the cancelling etc.

I thought about it a bit, and I think I have an idea on how to make this work.
A lot of this comes from anticipating similar needs in the future for usage events and other detail views.

I basically want to have a popup template, and be able to use either the full page or the popup template based on a given request parameter. i.e. http://nemo.nist.gov/event_details/reservation/1/ -> full page, http://nemo.nist.gov/event_details/reservation/1/?popup_view=true -> popup

I realize this is slightly outside of the scope of this feature, but if you are open to it, here is what I propose:

  1. Change the original reservation details page with your styling (because I like it). and make sure it looks good in the popup.
  2. Add a popup.html template in templates/base folder with the following code:
<div class="modal-header">
    <button type="button" class="close" data-dismiss="modal">&times;</button>
    <h4 class="modal-title">{% block title %}{% endblock %}</h4>
</div>
<div class="modal-body">
    {% block content %}{% endblock %}
</div>
  1. At the top of the reservation_details page, add {% extends popup_view|yesno:"base/popup.html,base.html" %}
  2. Move the current reservation_details function from calendar.py into your details.py
  3. Rename the file event_details.py
  4. Move the current reservation details url in urls.py to a separate section called # Event details right after the # Calendar section
  5. Change the reservation details function to get a popup_view from the request: popup_view = request.GET.get("popup_view") and pass it to the template.
  6. In reservation_event_feed.html, specific_user_feed.html and usage_event_feed.html change "{% url 'reservation_details' x.id %}" to "{% url 'reservation_details' x.id %}?popup_view=true"
  7. Move the templates/calendar/reservation_details.html file to a new folder templates/event_details/reservation_details.html
  8. Finally, if you want the change title, etc. functions to work, add the js in reservation_details.html with an {% if not popup_view %} otherwise change the conditions to only editing if not popup_view. I can go either way with having that functionality in there or not.
  9. Make sure everything works (I didn't test any of it)

If realize it's a lot, so it's also totally fine if you just do 1) and I'll do the rest.

Implemented using query parameter `popup_view`; which if true, will format
things as modal and use the new base/popup.html template. If the parameter is
not given or false, the template will use the "standalone" format and return
a full featured page extended from the base.html template
@jat255
Copy link
Member Author

jat255 commented Dec 17, 2021

Ok, I believe I've implemented everything you suggested, as well as changing how the reservation question groups are implemented in the template (I did it using a templatetag, since it was easier for me to do in Python than in the Django template language).

There are two remaining issues/things I wanted to point out:

  • if viewing details from a modal, and you do something that fails (trying to change project of a past reservation, for example); the modal response from the failed callback doesn't show up until the first modal is closed (since you can only have one modal open at a time...)
  • to make the the new table design fit in the modal, I had to make the modal "large" size. This is done by adding modal-lg to the modal-dialog class, but this is set in the base template, which means all modals will now be modal-lg (unless we pulled this out into the popup.html, maybe). If this is a problem, we should refactor it a bit

@rptmat57
Copy link
Contributor

adding the "data-dismiss" attribute on the buttons takes care of the issue with the new popup not appearing.

for the large popup, I'll take care of it. I would prefer it only be large for this, since all error messages etc. look a bit silly in the big version

@jat255
Copy link
Member Author

jat255 commented Dec 17, 2021

Ok, cool. Let me know if you need any changes from me. You should be able to push to my branch directly, I think

Copy link
Contributor

@rptmat57 rptmat57 left a comment

Choose a reason for hiding this comment

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

alright if you can address those things that should do it.
I'll fix the popup size separately
thanks! sorry this got a bit crazy

NEMO/templates/event_details/reservation_details.html Outdated Show resolved Hide resolved
NEMO/views/calendar.py Outdated Show resolved Hide resolved
NEMO/views/event_details.py Outdated Show resolved Hide resolved
NEMO/views/event_details.py Show resolved Hide resolved
NEMO/views/event_details.py Outdated Show resolved Hide resolved
NEMO/templates/event_details/reservation_details.html Outdated Show resolved Hide resolved
NEMO/templates/event_details/reservation_details.html Outdated Show resolved Hide resolved
NEMO/templates/event_details/reservation_details.html Outdated Show resolved Hide resolved
NEMO/templates/event_details/reservation_details.html Outdated Show resolved Hide resolved
NEMO/templates/event_details/reservation_details.html Outdated Show resolved Hide resolved
@jat255
Copy link
Member Author

jat255 commented Dec 17, 2021

Oh, I see your comments now. I'll take care of those shortly

@rptmat57
Copy link
Contributor

most of them can be pushed directly from the suggestions.
there is a couple more that you'll need to address

Copy link
Member Author

@jat255 jat255 left a comment

Choose a reason for hiding this comment

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

These were addressed in a separate commit (I didn't feel comfortable with knowing how to do it directly from Github, so sorry if that messed up the code-review process)

@jat255 jat255 requested a review from rptmat57 December 17, 2021 17:29
Copy link
Contributor

@rptmat57 rptmat57 left a comment

Choose a reason for hiding this comment

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

all good, thanks for being accommodating!!

@rptmat57 rptmat57 changed the base branch from master to 3.14.0.dev December 17, 2021 20:13
@rptmat57 rptmat57 merged commit 55810bf into usnistgov:3.14.0.dev Dec 17, 2021
@jat255 jat255 deleted the FEAT_model_landing_pages branch December 17, 2021 20:16
jat255 added a commit to usnistgov/NexusLIMS that referenced this pull request Jul 16, 2022
Note: we may need to tweak the NEMO implementation depending on the final
implementation of usnistgov/NEMO#97

Also still need to modify XSLT to match
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.

[Feature request] Have dedicated landing pages for reservations
2 participants