-
Notifications
You must be signed in to change notification settings - Fork 3
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
Story/vspc 190 A user should be able to define the order spaces appear in the site menu of the public page #277
Conversation
…y user instead of drag and drop
vspace/src/main/java/edu/asu/diging/vspace/core/model/ExhibitionSpaceOrderMode.java
Show resolved
Hide resolved
return value; | ||
} | ||
|
||
public static List<String> getAllValues() { |
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 can probably just get rid of the values, and simply pass ALPHABETICAL
or CREATION_DATE
as request parameter. Then you can use the valueOf
method to get an enum.
vspace/src/main/java/edu/asu/diging/vspace/core/services/impl/ExhibitionSpaceOrderUtility.java
Show resolved
Hide resolved
vspace/src/main/java/edu/asu/diging/vspace/core/services/impl/ExhibitionSpaceOrderUtility.java
Show resolved
Hide resolved
* @param publishedSpaces | ||
* @return list of {@link ISpace} | ||
*/ | ||
private List<ISpace> sortSpacesOnCreationDate(List<ISpace> publishedSpaces){ |
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.
see above
attributes.addFlashAttribute("alertType", "danger"); | ||
attributes.addFlashAttribute("message", "Custom order could not be updated. The title field is empty."); | ||
attributes.addFlashAttribute("showAlert", "true"); | ||
return "redirect:/staff/space/order/"+spacesCustomOrderId; |
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.
same as before, data should not be lost.
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.
?
@Autowired | ||
private ISpacesCustomOrderManager spacesCustomOrderManager; | ||
|
||
@RequestMapping(value = "/staff/space/order/{customOrderId}/edit/info", method = RequestMethod.POST) |
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.
and then this url can just be /staff/space/order/{customOrderId}
} | ||
|
||
@RequestMapping(value = "/staff/space/order/{customOrderId}/edit/spaceorders", method = RequestMethod.POST) | ||
public String saveOrder( |
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.
is there are reason, name/description and order are not stored together in one call?
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.
right now, only the order can be changed on the edit page, and on the same page, there is a separate option to edit the name/description, which opens a popup to edit and save only the name/description.
Should we keep it all together (name, description and order) in one page?
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.
yes, I don't see a reason not to
*/ | ||
|
||
@Controller | ||
public class StaffSpaceCustomOrderController { |
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.
name should make clear what this controller is for
import edu.asu.diging.vspace.core.services.ISpacesCustomOrderManager; | ||
|
||
@Controller | ||
public class UpdateStaffSpaceOrderModeController { |
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.
why is this called ...StaffSpace...
not just ...Space...
? There is not equivalent for the public page, right? This controller does not need to be contrasted with another one for the public site?
.getSpacesCustomOrder(); | ||
if(spaceCustomOrder!=null && spaceCustomOrder.getId().equals(id)) { | ||
exhibition.setSpacesCustomOrder(null); | ||
exhibitionManager.storeExhibition((Exhibition)exhibition); |
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.
?
attributes.addFlashAttribute("alertType", "danger"); | ||
attributes.addFlashAttribute("message", "Custom order could not be saved. Please enter the name of the order."); | ||
attributes.addFlashAttribute("showAlert", "true"); | ||
return "redirect:/staff/space/order/add"; |
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.
?
attributes.addFlashAttribute("alertType", "danger"); | ||
attributes.addFlashAttribute("message", "Custom order could not be updated. The title field is empty."); | ||
attributes.addFlashAttribute("showAlert", "true"); | ||
return "redirect:/staff/space/order/"+spacesCustomOrderId; |
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.
?
} | ||
|
||
@RequestMapping(value = "/staff/space/order", method = RequestMethod.GET) | ||
public String displayCustomOrders(Model model) { |
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.
?
} | ||
|
||
@RequestMapping(value = "/staff/space/order/setExhibitionCustomOrder", method = RequestMethod.POST) | ||
public String setExhibitionSpacesCustomOrder(Model model, |
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.
this should be in the same class as the one to show the page for a specific order, but url should be /staff/space/order/{orderId}
.
Resolve conflicts please |
# Conflicts: # vspace/src/main/java/edu/asu/diging/vspace/core/services/ISpaceManager.java # vspace/src/main/java/edu/asu/diging/vspace/core/services/impl/SpaceManager.java
@@ -69,6 +69,7 @@ public void setExhibition(JoinPoint jp) { | |||
* spaces with null space status | |||
*/ | |||
publishedSpaces.addAll(spaceManager.getSpacesWithStatus(null)); | |||
publishedSpaces = spaceManager.sortPublishedSpacesByGivenOrder(publishedSpaces); |
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.
since sortPublishedSpacesByGivenOrder
returns a list, there is really no need to add all spaces to publishedSpaces
first, is there? Can't this be replaced with:
publishedSpaces = spaceManager.sortPublishedSpacesByGivenOrder(spaceManager.getSpacesWithStatus(null));
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.
publishedSpaces has spaces with spaceStatus null and PUBLISHED, did not change it as I thought both the type of spaces are needed. Should I change the existing logic?
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.
ah, GitHub hid the the other line above before, so I missed it. The way it was was correct
private IExhibitionManager exhibitionManager; | ||
|
||
/** | ||
* This method sorts the spaces in alphabetical order. |
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.
There should probably be a comment that this method orders spaces in place, so that the passed in list of spaces will be sorted after the method is done.
} | ||
|
||
/** | ||
* This method sorts the spaces based on the space creation date. |
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.
same here
|
||
|
||
/** | ||
* This method sorts the spaces based on user defined order. |
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.
here on the other hand, this is only the case if the space custom order is null. Otherwise, the space list passed will stay the way it was.
Make it so, Jenkins. |
After that should be good. |
Guidelines for Pull Requests
If you haven't yet read our code review guidelines, please do so, You can find them here.
Please confirm the following by adding an x for each item (turn
[ ]
into[x]
).Please provide a brief description of your ticket
(you can copy the ticket if it hasn't changed)
...Put description here...
Right now, on the public site, the menu simply lists all published spaces. The user has no control over the order. For this ticket two things should be implemented:
by default the spaces should be ordered alphabetically by name
a user should be able to switch between alphabetically ordered, ordered by creation date, and a custom order.
For the custom order, there should be a page (on the staff side) that lists all spaces and the user can simply arrange them in the order they should be shown.
Anything else the reviewer needs to know?
User can choose if they have to arrange spaces alphabetically or through creation date or a custom order in spaces on the staff site.
They can create custom orders in the custom order page on the staff site. Alphabetical is default, if user chooses custom but does not select an order it will fall back to alphabetical order.