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

[HOLD] Add display messages #2045

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[HOLD] Add display messages #2045

wants to merge 1 commit into from

Conversation

jcoyne
Copy link
Collaborator

@jcoyne jcoyne commented May 15, 2020

[on hold until administrative permissions are worked out]

Why was this change made?

Add Home page messages. Fixes #924

How was this change tested?

Which documentation and/or configurations were updated?

@ndushay ndushay changed the title Content blocks [DRAFT] Content blocks May 16, 2020
@jcoyne jcoyne force-pushed the content-blocks branch 5 times, most recently from 115e41a to f2cc353 Compare May 16, 2020 21:22
@jcoyne jcoyne changed the title [DRAFT] Content blocks Content blocks May 16, 2020
@jcoyne jcoyne marked this pull request as ready for review May 16, 2020 21:23
@jcoyne jcoyne force-pushed the content-blocks branch 3 times, most recently from 0971822 to 0d8aa85 Compare May 16, 2020 22:04
@jcoyne jcoyne changed the title Content blocks Add display messages May 16, 2020
Copy link
Member

@jmartin-sul jmartin-sul left a comment

Choose a reason for hiding this comment

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

some nitpicky questions, overall looks good to me (but also i'd be hesitant to merge myself, since i'm not very familiar with stimulus, and since there aren't screenshots and i haven't tested, though i'm sure you have/will on stage)

app/javascript/style/variables.scss Outdated Show resolved Hide resolved
app/models/content_block.rb Show resolved Hide resolved
spec/models/content_block_spec.rb Outdated Show resolved Hide resolved
spec/requests/content_blocks_spec.rb Show resolved Hide resolved
@jcoyne jcoyne force-pushed the content-blocks branch 3 times, most recently from 6329bd2 to 9ac26e8 Compare May 20, 2020 20:31
@jcoyne jcoyne force-pushed the blacklight-7-hold branch from c8472c8 to 5b1be08 Compare May 22, 2020 13:26
@jcoyne jcoyne force-pushed the blacklight-7-hold branch 4 times, most recently from 65bf0fa to 1a9ad6a Compare June 4, 2020 04:43
@jcoyne jcoyne force-pushed the blacklight-7-hold branch from de1b1c6 to 953062e Compare June 4, 2020 21:04
@jcoyne jcoyne force-pushed the blacklight-7-hold branch from 953062e to 66598ad Compare June 5, 2020 19:13
@jcoyne jcoyne force-pushed the blacklight-7-hold branch from 66598ad to 34b6d07 Compare June 5, 2020 21:33
@jcoyne jcoyne force-pushed the blacklight-7-hold branch from 34b6d07 to 92e2b6b Compare June 8, 2020 16:36
@jcoyne jcoyne force-pushed the blacklight-7-hold branch 3 times, most recently from ceab673 to fe56ee0 Compare June 10, 2020 00:10
@jcoyne jcoyne force-pushed the blacklight-7-hold branch from fe56ee0 to b923314 Compare June 22, 2020 15:31
Base automatically changed from blacklight-7-hold to master June 22, 2020 16:00
@jcoyne jcoyne force-pushed the content-blocks branch 2 times, most recently from 3e164aa to 9f54785 Compare June 29, 2020 22:34
@andrewjbtw
Copy link

@jcoyne I've created a specific workgroup for being able to edit these messages: sdr:argo-message-editors

@jcoyne
Copy link
Collaborator Author

jcoyne commented Jul 6, 2020

@andrewjbtw the problem is that prior to this pull request, we have given members of sdr:administrator-role the ability to do everything in Argo. Are you asking me to remove that ability? Do you want to create workgroups for all other types of access?

Copy link
Contributor

@ndushay ndushay left a comment

Choose a reason for hiding this comment

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

can we get an image of what these changes look like?

@jcoyne
Copy link
Collaborator Author

jcoyne commented Jul 7, 2020

@ndushay on the issue: #924 (comment)

@andrewjbtw
Copy link

So unfortunately, we're going to have to put this on hold until administrative permissions are worked out, which might not be for a long time. The problem with this feature vs. other features people have admin access to is that these messages are displayed prominently on the front page for everyone and too many (100+) people would have full edit access under the current permissions scheme. The risk is low that people would abuse their access, but it's the kind of thing where it would be hard to explain why it was left open if something did happen.

It's true that admin access means that many users could cause other types of problems, but I don't think any other change people can make would be displayed to all users as soon as they load Argo, and there are other permissions (like APOs) that limit the scope of what people can do. What we really need to do is rationalize permissions, which is something I'll be working on, first by mapping out the current workgroup situation. As I understand it, Argo was originally intended to be adminstrator-only, with not a large user base, which is probably why people kept getting added to the admin roles.

@jcoyne jcoyne changed the title Add display messages [HOLD] Add display messages Jul 7, 2020
@mjgiarlo mjgiarlo changed the base branch from master to main January 8, 2021 19:21
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.

Support Argo home page message or alert
4 participants